-
Notifications
You must be signed in to change notification settings - Fork 51
tests: Update cache dir test to not build different distro #948
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request refactors the cache directory test to avoid building multiple distros within a single test, enabling better CI job parallelization by allowing each distro to be tested in separate jobs.
Changes:
- Introduces a build tag mechanism (
alt_testing_targets) that conditionally registers duplicate distro implementations with an "alt" suffix (e.g., "jammy" and "jammyalt") - Modifies the Dockerfile to accept an
EXTRA_BUILD_FLAGSARG for passing build tags - Updates the test infrastructure to build the frontend with the alt_testing_targets tag enabled
- Refactors the cache test to use the alt target instead of building a completely different distro
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
Dockerfile |
Adds EXTRA_BUILD_FLAGS ARG and applies it to the go build command for the frontend |
test/testenv/build.go |
Passes -tags alt_testing_targets via build-arg when building the frontend for tests |
internal/frontendapi/router.go |
Adds logic to register alt target keys when the build tag is enabled, adds AltTargetKey helper function |
internal/frontendapi/alt_targets.go |
New file with build tag constraint that enables alt testing targets |
internal/frontendapi/no_alt_targets.go |
New file with build tag constraint that disables alt testing targets by default |
test/cache_test.go |
Refactors checkDistro function to use alt target instead of hardcoded different distro, removes unused path import |
This test was always building a distro that is not the current distro under test for the purpose of validating cache key generation. This is undesirable for CI because we want to split each distro into separate jobs. Before this change the the job for, e.g., testing jammy would also build noble. This change adds a build tag that we use for tests that injects the same distro implementation twice with but with a modified target key name. e.g. adds "jammy" and "jammyalt". This allows us to test the same case but without having to build an entirely different distro stack. Signed-off-by: Brian Goff <[email protected]>
435ff97 to
7d38733
Compare
invidian
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR seems like a lot of hacking and custom code just to support a single test. Do you think this indicates a bigger problem with code which would need a better long-term solution? Maybe it would be better to turn off this test temporarily until we have a better solution? Or maybe this specific test could be moved to the Other suite, rather than running for every distro?
Unfortunately, as a still fresh person to the project, I am not able to propose an alternative solution at the moment, but I would be happy to look for a better solution.
|
I do think we need to look at how we setup each distro. That whole abstraction needs an overhaul. |
This test was always building a distro that is not the current distro under test for the purpose of validating cache key generation. This is undesirable for CI because we want to split each distro into separate jobs.
Before this change the the job for, e.g., testing jammy would also build noble.
This change adds a build tag that we use for tests that injects the same distro implementation twice with but with a modified target key name. e.g. adds "jammy" and "jammyalt".
This allows us to test the same case but without having to build an entirely different distro stack.