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

Move cluster package e2e tests to integration tests #341

Merged
merged 2 commits into from
Sep 14, 2023

Conversation

pablochacin
Copy link
Collaborator

@pablochacin pablochacin commented Sep 12, 2023

Description

Moves the cluster tests to the package as integration tests. The are no changes to the tests.

Checklist:

  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works.
  • I have run linter locally (make lint) and all checks pass.
  • I have run tests locally (make test) and all tests pass.
  • I have run relevant integration test locally
  • I have run relevant e2e test locally (make e2e-xxx for disruptors, kubernetes or cluster related changes)
  • Any dependent changes have been merged and published in downstream modules

@pablochacin pablochacin changed the title Move cluster e2e to package Move cluster package e2e tests to integration tests Sep 13, 2023
Signed-off-by: Pablo Chacin <[email protected]>
@pablochacin pablochacin marked this pull request as ready for review September 13, 2023 17:47
Copy link
Member

@roobre roobre left a comment

Choose a reason for hiding this comment

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

I'm not 100% convinced about this change. I agree that these are technically integration tests, but they are integration tests that test and use the e2e suite, and thus have the requirements and behavior of the e2e suite.

My expectation for when I run an integration test (e.g. make integration-xxx, or go test -tags integration) is that it will behave similarly to the rest of integration tests we have, which is not the case here.

Are we getting any other benefit from this change other than more proper categorization?

@pablochacin
Copy link
Collaborator Author

@roobre these tests do not use the e2e test framework at all. In fact, the cluster package is used by the e2e test framework.

The benefit is to separate the tests that actually use the e2e framework (basically, the disruptor test) from the tests that target one single component, like this one.

I have changed the name of the test clusters to avoid any reference to e2e, in case this may induce confusion.

@roobre
Copy link
Member

roobre commented Sep 14, 2023

Makes sense, I think what confused me was that both used kind-related things under the hood.

@pablochacin
Copy link
Collaborator Author

I think what confused me was that both used kind-related things under the hood.

The purpose of this package is to isolate the e2e tests from the actual test infrastructure we use (in this case, kind). So is not that they both use the same infrastructure but this cluster package provides the infrastructure to the e2e tests. The tests we are moving here just test the integration with kind.

@pablochacin pablochacin merged commit a986f26 into main Sep 14, 2023
6 checks passed
@pablochacin pablochacin deleted the move-cluster-e2e-tests branch September 14, 2023 12:43
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.

2 participants