Add more tests to building (deb) containers #945
Conversation
invidian
force-pushed
the
invidian/more-container-tests
branch
5 times, most recently
from
a575037 to
64051d8
Compare
invidian
changed the title
invidian
force-pushed
the
invidian/more-container-tests
branch
4 times, most recently
from
d74af94 to
54d7406
Compare
| t.Fatalf("Unexpected error extracting LLB OPs from state: %v", err) | ||
| } | ||
|
|
||
| cacheIgnored := 0 |
There was a problem hiding this comment.
To make this test more robust, we could modify the build step to include the timestamp when building a package and to generate a timestamp when package is installed. This way, we could assert that the package building timestamp does not change between runs, but installation timestamp does.
| Steps: []dalec.BuildStep{ | ||
| { | ||
| Command: ` | ||
| # This is not a debian build, skip this. |
There was a problem hiding this comment.
We should be able to add a separate step for RPM-based builds to also cover RPM implementation with this test.
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive unit tests for DEB container builds and refactors helper functions to reduce code duplication. The tests verify various aspects of container building including base image handling, package installation, cache management, and repository configuration.
Changes:
- Added unit tests in
targets/linux/deb/distro/container_test.goto verify container building behavior without requiring full integration tests - Added integration tests in
test/linux_target_test.goto verify cache handling and repository mounting during container builds - Refactored LLB operation inspection functions from test-specific code to shared helpers in
helpers.go
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
targets/linux/deb/distro/container_test.go |
New unit test file with comprehensive tests for BuildContainer function, including base image selection, package installation order, and apt cache mounting |
test/linux_target_test.go |
Added integration tests for cache key handling, dpkg debug configuration, upgrade settings, and Ubuntu-specific dpkg excludes handling |
test/helpers_test.go |
Removed duplicate LLB operation helper functions and migrated to use shared functions from dalec package |
helpers.go |
Added LLBOpsFromState, LLBOpsToJSON, and LLBOp type to provide shared functionality for inspecting LLB operations |
targets/linux/deb/distro/container.go |
Refactored BuildContainer to simplify base image logic, remove duplicate apt cache mount, and improve code organization |
frontend/request.go |
Simplified IgnoreCache function by replacing manual string parsing with strings.SplitSeq |
invidian
force-pushed
the
invidian/more-container-tests
branch
from
54d7406 to
c3e66c0
Compare
cpuguy83
left a comment
cpuguy83
left a comment
There was a problem hiding this comment.
Ia this still a work in progress?
…output image It is not user configurable, so if that condition occurs, it's a bug in dalec target specificiation, so we expect tests to fail and catch that. Also this code will not be relevant once we switch to minimal images. Signed-off-by: Mateusz Gozdek <[email protected]>
invidian
force-pushed
the
invidian/more-container-tests
branch
from
c3e66c0 to
35af7a1
Compare
| } | ||
| }) | ||
|
|
||
| t.Run("with_mounted_apt_cache", func(t *testing.T) { |
There was a problem hiding this comment.
I guess that could have an integration tests which does not rely on LLB inspection, but we would have to run build for all targets and verify that cache is empty for each target and that it is persistent across different runs.
And also verify that dpkg/apt actually writes to those paths something we expect to cache. That seem like a lot of work though.
Signed-off-by: Mateusz Gozdek <[email protected]>
invidian
force-pushed
the
invidian/more-container-tests
branch
from
35af7a1 to
5175083
Compare
cpuguy83
left a comment
cpuguy83
left a comment
There was a problem hiding this comment.
Do we need t.Parallel for these?
I'd expect these to be fast and likely that t.Parallel slows things down a bit.
|
|
||
| for _, op := range ops { | ||
| e := op.Op.GetExec() | ||
| if e == nil || op.OpMetadata.ProgressGroup.Name != "Install base image packages" { |
There was a problem hiding this comment.
Checking these look good, but perhaps it should be referencing a constant (both here and where we create the group)?
There was a problem hiding this comment.
I believe a general rule of thumb would be to not use constants in tests, so that tests can act as a test doubles, so if the constant change, you know what gets affected by it, which makes it more intentional. I know this specific case is low impact since it's only about the progress group, but it still sets an example which may be followed for more important constants in the future.
Let me know if you still want me to convert that to constant.
Are you referring to the unit tests? I think making all tests parallel by default is a good practice, since even if you slow down some tests, you are likely to significantly speed up slower tests. + it better enforces not mutating global variables, which helps making sure code does not have implicit global dependencies etc. There are even linters which verifies that. |
2 participants
What this PR does / why we need it:
As part of #448, to minimize a chance for regression once we re-implement deb container building to use minimal images, this PR adds additional tests to deb container building to cover untested parts of code. New tests can easily be extended to cover RPM containers as well.
Some tests cannot be written as integration tests, hence they are written as unit tests for container building.
Which issue(s) this PR fixes (optional, using
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when the PR gets merged):Part of #448