Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow bridged providers to opt-in to sharding #1217

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

blampe
Copy link
Contributor

@blampe blampe commented Dec 12, 2024

Extracts the sharding part of #1140 on
top of the test consolidation in #1214.

Testing this in pulumi/pulumi-aws#4912.

The template's test workflow gets messy because sharded tests need to setup all
SDKs instead of individual ones.

Sharding logic has been forked to http://github.com/pulumi/shard so anyone can contribute to it while I'm out. (It pains me that it's public, but GitHub requires forks to stay public for some reason.)

Fixes #190

@blampe
Copy link
Contributor Author

blampe commented Dec 12, 2024

This change is part of the following stack:

Change managed by git-spice.

@blampe blampe force-pushed the blampe/test-consolidate branch from 94f51fa to ef98e62 Compare December 12, 2024 01:10
Base automatically changed from blampe/test-consolidate to master December 12, 2024 23:01
@blampe blampe force-pushed the blampe/bridged-sharding branch from e25bbdd to 4f9db6c Compare December 13, 2024 01:30
Copy link
Member

@danielrbradley danielrbradley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CI side of things looks good, but I would strongly advocate for not adding multiple CI-specific targets in the makefile. Added some alternative design ideas inline.

Comment on lines +383 to +384
# shard computes tests to run and modifies the CI runner's environment.
shard:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't use $(GITHUB_ENV) to modify the environment as that won't be runnable locally. I really think we want to leverage the single target that's integrated via the existing test target which might be a little more fiddly to write but makes it easier for the caller to repro exactly what CI did.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally agree with everything you're saying but would prefer to make those optimizations in a followup. It's nice that we manage the Makefile for these bridged providers because we're not locked in to anything here.

It is possible to repro things locally by looking up the shard information

Run make test_shard
env:
    SHARD_TESTS: ^(?:TestACMUpgrade|TestAccLogGroup|TestAccRoute53|TestAccServerlessRaw|TestAccWebserverGo|TestEcrLifecyclePolicyUpgrade|TestIMDSAuth|TestLogGroupUpgrade|TestRdsInstanceUpgrade|TestRegress3421Update|TestRegress4128|TestSecretManagerPy)\$
    SHARD_PATHS: ./.

and you can generate that information yourself if you really want to (GITHUB_ENV=env SHARD_TOTAL=10 SHARD_INDEX=2 make shard && cat env).

So you can in theory run SHARD_TESTS=... SHARD_PATHS=... make test_shard locally. But in practice there are so many other environment variables I don't think this will actually be feasible until we've done something like #1207.

Comment on lines +69 to +78
#{{- range $_, $language := .Config.Languages }}#
- name: Download #{{ $language }}# SDK
uses: ./.github/actions/download-sdk
with:
language: #{{ $language }}#
- name: Restore makefile progress
run: make --touch provider schema build_#{{ $language }}#
- name: Install dependencies
run: make install_#{{ $language }}#_sdk
#{{- end }}#
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can make this less repetitive in the generated code by doing the touch and install outside the loop and leveraging the phony grouping targets.

Suggested change
#{{- range $_, $language := .Config.Languages }}#
- name: Download #{{ $language }}# SDK
uses: ./.github/actions/download-sdk
with:
language: #{{ $language }}#
- name: Restore makefile progress
run: make --touch provider schema build_#{{ $language }}#
- name: Install dependencies
run: make install_#{{ $language }}#_sdk
#{{- end }}#
#{{- range $_, $language := .Config.Languages }}#
- name: Download #{{ $language }}# SDK
uses: ./.github/actions/download-sdk
with:
language: #{{ $language }}#
#{{- end }}#
- name: Restore makefile progress
run: make --touch provider schema build_sdks
- name: Install dependencies
run: make install_sdks

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally, we could allow the "Download SDK" action to accept multiple languages (e.g. comma separated or something) to make the call sight cleaner and more understandable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally forgot about install_sdks, good call!

@@ -258,7 +258,7 @@ bin/$(PROVIDER): .make/schema

test: export PATH := $(WORKING_DIR)/bin:$(PATH)
test:
cd examples && go test -v -tags=all -parallel $(TESTPARALLELISM) -timeout 2h
cd #{{ .Config.E2ETestDirectory }}# && go test -v -tags=all -parallel $(TESTPARALLELISM) -timeout 2h
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternative design - proxy all test runs through your sharded test runner which could then apply the sharding in a single step, if needed:

Suggested change
cd #{{ .Config.E2ETestDirectory }}# && go test -v -tags=all -parallel $(TESTPARALLELISM) -timeout 2h
cd #{{ .Config.E2ETestDirectory }}# && go run github.com/pulumi/go-test-shard@v1 --total "$(SHARD_TOTAL)" --index "$(SHARD_INDEX)" -- -v -tags=all -parallel $(TESTPARALLELISM) -timeout 2h

The exact same change could be applied to the provider_test target too and could be moved from the prerequisites to the test matrix too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The exact same change could be applied to the provider_test target too and could be moved from the prerequisites to the test matrix too.

I've been thinking of provider_test as essentially unit tests which make sense to keep in prerequisites as a lightweight sanity check before the heavy tests start.

See my comment here about integration tests in provider. Basically our poor module structure is pushing us towards some awkward conventions, and there are a ton of benefits to fixing the underlying issue instead of catering to those conventions.

By default we can expect "slow" tests to live in one directory, but this doesn't have to be a requirement as long as the provider can eventually customize targets. With a root-level go.mod it becomes trivial to run integration tests across any number of directories -- in other words it's possible to specify SHARD_PATHS="./examples ./provider" when they belong to the same module.

@blampe blampe requested a review from t0yv0 January 3, 2025 23:40
This reverts commit 635117e.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Save build time by adding an option to break acceptance tests down further
2 participants