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

Refactor kpack-image-builder/controllers/builderinfo_controller_test.go #3743

Open
danail-branekov opened this issue Jan 22, 2025 · 1 comment
Labels

Comments

@danail-branekov
Copy link
Member

danail-branekov commented Jan 22, 2025

  • This test is terrible in so many ways!
  • It is Serial, i.e. does not run in parallel

Let's fix that

@github-project-automation github-project-automation bot moved this to 🧊 Icebox in Korifi - Backlog Jan 22, 2025
danail-branekov added a commit that referenced this issue Jan 24, 2025
The builder info is created in `JustBeforeEach` so that the test could
conifugure its finalizers in `BeforeEach`. This is to address a flake in
the test when the builder info is being deleted, e.g
https://ci.korifi.cf-app.com/teams/main/pipelines/main/jobs/run-tests-periodic/builds/21592

While being here,
- refactor the suite to create the test root namespace and start the controller in `BeforeEach`. Thus test could not bother cleaning up.
- remove`Serial` from the builder info controller test
- Simplify the builder info controller test by removing contexts that do
  not contribute changes to the default setup
- Remove test cases that verify that the builder info controller is
  listening on cluster builder eventually deletion. The test verifies
  that the info is reconciled on builder change, no need to check the
  same on cluster builder delete (as from controller's point of view,
  the change type does not really matter)

issue #3743
@danail-branekov
Copy link
Member Author

While PR #3753 introduces certain improvements, the test is still not great

georgethebeatle pushed a commit that referenced this issue Jan 27, 2025
The builder info is created in `JustBeforeEach` so that the test could
conifugure its finalizers in `BeforeEach`. This is to address a flake in
the test when the builder info is being deleted, e.g
https://ci.korifi.cf-app.com/teams/main/pipelines/main/jobs/run-tests-periodic/builds/21592

While being here,
- refactor the suite to create the test root namespace and start the controller in `BeforeEach`. Thus test could not bother cleaning up.
- remove`Serial` from the builder info controller test
- Simplify the builder info controller test by removing contexts that do
  not contribute changes to the default setup
- Remove test cases that verify that the builder info controller is
  listening on cluster builder eventually deletion. The test verifies
  that the info is reconciled on builder change, no need to check the
  same on cluster builder delete (as from controller's point of view,
  the change type does not really matter)

issue #3743
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: 🧊 Icebox
Development

No branches or pull requests

1 participant