Fix apparent typos in zstd:chunked tests#24686
Fix apparent typos in zstd:chunked tests#24686openshift-merge-bot[bot] merged 3 commits intocontainers:mainfrom
Conversation
Luap99
left a comment
There was a problem hiding this comment.
If this change did not cause the test to fail are we testing the right thing? Clearly this option does not seem to matter so why are we even setting it?
we force to force the pull to the "partial pulls" code path even with traditional images. We can improve the test to really check that it is what happens (and we could have caught the issue earlier doing so), but we need that flag to test the conversion code path. |
Well we must do that, without it the test seems like a NOP. If you want to test the "partial pulls" code path then the test MUST check that this code path is used. Passing the right cli option is not good enough. What if the option passing has a bug or we do not pass the option along the right code paths, etc...? If the test didn't fail before it will not fail when a regression causes the code path to not be executed. |
|
@mtrmac could you please amend something like? diff --git a/test/system/155-partial-pull.bats b/test/system/155-partial-pull.bats
index 961354bc7..e1673eb4a 100644
--- a/test/system/155-partial-pull.bats
+++ b/test/system/155-partial-pull.bats
@@ -84,9 +84,10 @@ EOF
run_podman $globalargs rmi $image2 $image3
- run_podman $globalargs pull \
+ run_podman --log-level debug $globalargs pull \
$pushpullargs \
$image2
+ assert "$output" =~ "Retrieved partial blob"
run -0 skopeo inspect containers-storage:$image2
assert "$output" =~ "application/vnd.oci.image.layer.v1.tar\+zstd" "pulled image must be zstd-compressed"
@@ -182,9 +183,10 @@ EOF
run_podman $globalargs rmi $image
- run_podman $globalargs pull \
+ run_podman --log-level debug $globalargs pull \
$pushpullargs \
$image
+ assert "$output" =~ "Retrieved partial blob"
# expect that the image contains exactly the same data as before
mount_image_and_take_digest $image |
That’s not what this option does, FWIW (that would be |
|
For the record: As of commit 466e1af , adding the checks for |
|
Totally unrelated, but: https://cirrus-ci.com/task/6587238856785920 reports success, but the detailed logs https://api.cirrus-ci.com/v1/task/6587238856785920/logs/main.log show 4 failed tests (all in the setup phase). Is that expected??? @Luap99 any idea? |
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Luap99
left a comment
There was a problem hiding this comment.
Totally unrelated, but: https://cirrus-ci.com/task/6587238856785920 reports success, but the detailed logs https://api.cirrus-ci.com/v1/task/6587238856785920/logs/main.log show 4 failed tests (all in the setup phase). Is that expected???
Uhm no, like this is bad, very bad... I need to do some tests there if bats didn't exit > 0 (bats problem) or if our script that runs bats twice ignores the exit code somehow (unlikely in my eyes as I have be seen many test failures since we changed that)
test/system/155-partial-pull.bats
Outdated
|
|
||
| function setup() { | ||
| skip_if_remote "none of these tests work with podman-remote" | ||
| skip_if_remote "zstd:chunked tests depend on start_registry, which does not work with podman-remote" |
There was a problem hiding this comment.
It is not just that,
--storage-driver, --pull-option enable_partial_pulls=true, --cert-dir are all gloab podman options that are not supported by the remote client as they need to be set on the server. But in sys test there is only one server for all tests.
There was a problem hiding this comment.
I found “does not work with podman-remote” 100% unhelpful, so I’m trying to capture reasons.
Is the current version better?
If Podman maintainers know what this is about and “does not work with podman-remote” is sufficient, I’m happy to leave things alone.
There was a problem hiding this comment.
Yes I agree skips must have a meaningful reason. skip_if_remote "doesn't work with remote" is pointless as skip_if_remote said as much. It should list a proper reason why not, i.e. what you did although the actual reason why start_registry doe snot work is that it uses gloabal storage options.
Basically any test using global podman options will not work correctly in a remote context.
test/system/155-partial-pull.bats
Outdated
| } | ||
|
|
||
| @test "zstd chunked does not modify image content" { | ||
| skip_if_remote "need to mount an image" |
There was a problem hiding this comment.
the first skip is part of setup() which is a magic bats function that will be run for each test case so this here is redundant
There was a problem hiding this comment.
Yes… I was trying to capture “this is one more reason why the test won’t work” but I appreciate that having a redundant skip here might be confusing.
I have split an attempt to reproduce that, + a bit of debug logging, into #24697 . I’d be perfectly happy to leave investigating this to an expert. |
I will chase this down, don't worry about it. Running this through several CI cycles is pretty pointless for me I can easily run the test locally. |
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
|
Updated:
Ready for review. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: giuseppe, Luap99, mtrmac The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
Does this PR introduce a user-facing change?