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

Consolidate integration tests #4910

Merged
merged 4 commits into from
Dec 12, 2024
Merged

Consolidate integration tests #4910

merged 4 commits into from
Dec 12, 2024

Conversation

blampe
Copy link
Contributor

@blampe blampe commented Dec 11, 2024

This moves integration tests currently under provider into the examples directory, and removes provider_test defined in extraTests.

These integration tests will now benefit from pulumi/ci-mgmt#1033 as a result, and #4911 is no longer necessary.

Copy link

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

Maintainer note: consult the runbook for dealing with any breaking changes.

@blampe blampe requested review from flostadler and corymhall and removed request for flostadler December 12, 2024 04:37
Copy link
Contributor

@flostadler flostadler left a comment

Choose a reason for hiding this comment

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

Generally I find it a bit odd that we put the tests (i.e. go files) into the examples/ directory. Couldn't we do the same thing we did for EKS and put all the testdata and programs into provider and reference the example programs from there?

Not blocking on this though. This change is a great improvement!

@blampe
Copy link
Contributor Author

blampe commented Dec 12, 2024

@flostadler good questions!

Generally I find it a bit odd that we put the tests (i.e. go files) into the examples/ directory.

Yeah, examples has always been a very weird directory.

Couldn't we do the same thing we did for EKS and put all the testdata and programs into provider and reference the example programs from there?

It doesn't matter where the integration tests live -- we currently hard-code it to examples but that will be configurable after pulumi/ci-mgmt#1217. So if you want integration tests to live under provider then go for it.

However I would add that for simplicity it's a good idea to keep slower-running integration tests separate from unit tests. So for example if unit tests live under provider you can run go test ./provider/... without needing to remember -short or build tags or whatnot. In many cases our providers have zero unit tests, which is probably why we've started putting e2e tests with the provider -- no unit tests to commingle with. I think that's a bad pattern, but again it's up to you as the maintainer. At the end of the day we will run "fast" tests on one CI runner, and "slow" tests across several CI runners, and how you make that distinction will be up to you.

I should mention another (self-inflicted) reason for putting integration tests under provider is that it puts them in the same module as the rest of your code, which is a side-effect of us creating separate modules under examples/go.mod and provider/go.mod. This is an anti-pattern, and if the modules are consolidated under one go.mod at the repo root then it really doesn't matter where the tests live because they'll always belong to the same module.

Finally I'll also add that after we stop sharding by language it will no longer be necessary to need build tags on your integration tests, which gives you a much nicer developer experience when you need to touch them.

@blampe blampe merged commit 35d54ab into master Dec 12, 2024
26 checks passed
@blampe blampe deleted the blampe/provider-tests branch December 12, 2024 18:30
Copy link

codecov bot commented Dec 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 0.00%. Comparing base (e1ae150) to head (9c25997).
Report is 1 commits behind head on master.

Additional details and impacted files
@@      Coverage Diff       @@
##   master   #4910   +/-   ##
==============================
==============================

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-merge-queue bot pushed a commit to pulumi/ci-mgmt that referenced this pull request Dec 12, 2024
* Updates `test-provider/aws/.ci-mgmt.yaml` to what's currently
checked-in.
* Consolidates test jobs into a shared workflow using
`run-acceptance-tests` as the starting point.

`extraTests` are included in the shared workflow mostly for simplicity.
This will break aws because those tests will specify `needs:` that are
no longer valid. We can fix that manually or just remove these extra
tests (pulumi/pulumi-aws#4910,
pulumi/pulumi-aws#4909).

No special treatment is given to TestPulumiExamples. We're not using
this outside of azure-native, and it should really just be treated like
any other integration test
#1211. If we don't want these
tests to run in certain workflows then we should expose some info to let
them `t.Skip` themselves.

Fixes #1034
@pulumi-bot
Copy link
Contributor

This PR has been shipped in release v6.65.0.

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.

3 participants