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

tests(e2e): introduce environment wrapper and test suites #4343

Merged
merged 7 commits into from
Aug 11, 2023

Conversation

phisco
Copy link
Contributor

@phisco phisco commented Jul 18, 2023

Description of your changes

This PR introduces the following changes:

  • introduces the concept of "Test Suite", see test/e2e/README.md for more details.
  • switches the CI job from a matrix built on "areas" to achieve some kind of parallelism, to one built on "test suites" (areas could still be added later on as an additional parameter to the matrix to improve times)
  • introduces a dedicated configuration struct, globally instantiated from the e2e package as E2EConfig
  • WARNING: switch e2e-framework to use unreleased versions, mainly due to the fix to label selection introduced in kubernetes-sigs/e2e-framework@b38dd95
  • adds a BeforeEachFeature checking that all features are defining the a label ascribing them to a specific test preset, to avoid having unselected features by mistake, this is due to the fact that e2e-framework doesn't modifying the feature from the provided function so there is no way to implicitly add a default value to all the features not having it set. However "explicit is better than implicit", so I feel it's better like this.
  • sets the e2e matrix jobs not to fail fast, to avoid having successful jobs being killed due to a flaky test from another suite of tests, we should not have them, but it's inevitable and in such cases having the other successful runs helps identifying whether it's an actual issue (all red) or due to flakiness.

Test suites allow to define custom groups of tests alongside required additional conditional setup steps and custom Crossplane installation options which currently allow to run both all the non alpha feature tests against the default Crossplane configuration, but also to run for each alpha feature all the basic and feature specific tests against a Crossplane installation with the selected alpha feature enabled from the start. We run the uninstall and upgrade tests as part of the basic suite, so we are still covering the "what happens to a running Crossplane instance if we enable/disable this alpha feature?". This will allow us to gain in the future the confidence required to promote alpha features to beta, knowing that none of the basic tests is failing.

AFAICT, test suites should transparently work with all current test selection modes, e.g. -test.run <regexp> or e2e-framework selection via labels/features/assessments.

TODO:

I have:

  • Read and followed Crossplane's contribution process.
  • Added or updated unit and E2E tests for my change
  • Run make reviewable to ensure this PR is ready for review.
  • [] Added backport release-x.y labels to auto-backport this PR if necessary.

@phisco phisco requested a review from pedjak July 18, 2023 16:46
@phisco
Copy link
Contributor Author

phisco commented Jul 18, 2023

FYI: I wanted to open an issue first, but while trying to validate my idea on a long train trip, I ended up mostly implementing the whole thing and to change my initial idea according to a few limitations from the e2e-framework and go testing framework in general. So in the end I decided it would have been easier to let the code speak directly, but feel free to challenge or discuss the whole approach as if it was a proposal instead 🙏

@phisco phisco marked this pull request as ready for review July 19, 2023 08:36
@phisco phisco requested review from a team as code owners July 19, 2023 08:37
@phisco phisco requested review from muvaf and lsviben July 19, 2023 08:37
@phisco phisco changed the title tests(e2e): introduce E2EConfig and presets tests(e2e): introduce E2EConfig and test suites Jul 19, 2023
Copy link
Contributor

@pedjak pedjak left a comment

Choose a reason for hiding this comment

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

LGTM

@phisco phisco force-pushed the e2e-alpha-all-2 branch 5 times, most recently from e81a918 to a0245d8 Compare July 24, 2023 15:38
@phisco phisco mentioned this pull request Jul 25, 2023
4 tasks
Copy link
Contributor

@lsviben lsviben left a comment

Choose a reason for hiding this comment

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

Nice improvements, thanks @phisco . Added one minor question

test/e2e/main_test.go Outdated Show resolved Hide resolved
@phisco phisco requested a review from negz August 4, 2023 10:14
@phisco
Copy link
Contributor Author

phisco commented Aug 4, 2023

I would like to have a feedback from @negz too when he's back.

Copy link
Member

@negz negz left a comment

Choose a reason for hiding this comment

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

A few questions, but I like the general direction here.

test/e2e/README.md Outdated Show resolved Hide resolved
test/e2e/README.md Outdated Show resolved Hide resolved
test/e2e/compSchemaValidation_test.go Outdated Show resolved Hide resolved
test/e2e/main_test.go Outdated Show resolved Hide resolved
test/e2e/main_test.go Outdated Show resolved Hide resolved
config.LabelTestSuite: []string{SuiteCompositionWebhookSchemaValidation, config.TestSuiteDefault},
}),
)
}
Copy link
Member

Choose a reason for hiding this comment

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

I've noticed that some other users of e2e-framework have a sub package per "suite". Would that approach make sense for us? I think in that world all of these init functions would become part of TestMain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried moving them to separate sub packages too in the beginning, but then you need to either inject the environment from the root to all the sub packages in some way or reinitialize it for each one of them or make it importable from the root package, which would mean moving it to a non _test.go file. I tried looking around before this iteration, but couldn't find applicable examples, we can definitely think about that for the next iteration.

test/e2e/compSchemaValidation_test.go Outdated Show resolved Hide resolved
test/e2e/config/config.go Outdated Show resolved Hide resolved
Signed-off-by: Philippe Scorsolini <p.scorsolini@gmail.com>
Signed-off-by: Philippe Scorsolini <p.scorsolini@gmail.com>
Signed-off-by: Philippe Scorsolini <p.scorsolini@gmail.com>
Signed-off-by: Philippe Scorsolini <p.scorsolini@gmail.com>
Signed-off-by: Philippe Scorsolini <p.scorsolini@gmail.com>
Signed-off-by: Philippe Scorsolini <p.scorsolini@gmail.com>
@phisco phisco changed the title tests(e2e): introduce E2EConfig and test suites tests(e2e): introduce environment wrapper and test suites Aug 9, 2023
test/e2e/main_test.go Outdated Show resolved Hide resolved
test/e2e/README.md Outdated Show resolved Hide resolved
Copy link
Member

@negz negz left a comment

Choose a reason for hiding this comment

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

This looks great, thank you!

Signed-off-by: Philippe Scorsolini <p.scorsolini@gmail.com>
@phisco phisco merged commit 45f7cbc into crossplane:master Aug 11, 2023
15 checks passed
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.

None yet

4 participants