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?
| @@ -0,0 +1,479 @@ | |||
| package distro_test | |||
There was a problem hiding this comment.
I'm not a fan of this pattern.
Any particular reason to make a separate package here?
There was a problem hiding this comment.
From testing docs:
If the file is in a separate "_test" package, the package being tested must be imported explicitly and only its exported identifiers may be used. This is known as "black box" testing.
This style enforces no internal code being used or tested in tests, which should be yielding more robust tests and easier to refactor implementation, since public API is a contract and all observable or modifiable behaviour should be accessible via public API.
I believe this is a good pattern to promote as a default and treat other cases like possible red flags and exceptions.
…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]>
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.
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