containerd / nerdctl

contaiNERD CTL - Docker-compatible CLI for containerd, with support for Compose, Rootless, eStargz, OCIcrypt, IPFS, ...
Apache License 2.0
7.98k stars 594 forks source link

TestBuildIsShareableForCompatiblePlatform unclear what the intent is #3451

Open apostasie opened 1 week ago

apostasie commented 1 week ago

Description

TestBuildIsShareableForCompatiblePlatform relies on AssertErrNotContains, which apparently does not test stderr, but stdout.

Since there is never anything on stdout in that case, none of these asserts are doing anything.

Furthermore, the test on a different architecture will only pass if emulation is enabled, but fail otherwise.

Put otherwise, the test right now is doing two things:

It is unclear to me what the test intent is.

Can we clarify?

Thanks.

Tagging @Shubhranshu153

Steps to reproduce the issue

na

Describe the results you received and expected

na

What version of nerdctl are you using?

main

Are you using a variant of nerdctl? (e.g., Rancher Desktop)

None

Host information

No response

apostasie commented 1 week ago

I will assume this was meant to test building for a "different from the host but currently emulated platform", and rewrite the test accordingly, including verification that we are able to emulate.

Shubhranshu153 commented 1 week ago

i can fix it, AssertErrNotContains -> from the name i always assumed it checks stderr, probably a simple redirection of everything to stdout would fix it.

the test checks if a cross build is happening then a tarball is created. so if the test are running on amd then that portion would run and test but if its running on arm it would get skipped.

apostasie commented 1 week ago

i can fix it, AssertErrNotContains -> from the name i always assumed it checks stderr, probably a simple redirection of everything to stdout would fix it.

Probably this method should go. It does not really serve a purpose, since what we are checking here is that the build is successful (exitcode 0 should be sufficient).

the test checks if a cross build is happening then a tarball is created. so if the test are running on amd then that portion would run and test but if its running on arm it would get skipped.

This is not what is happening right now. If you are on anything but armv7 (eg: arm64), the test will run, and will fail if emulation is not installed (which is the default). The test should first verify that we support building on something else than the current platform, then build for that (and we should remove the hardcoded arm check).

Shubhranshu153 commented 1 week ago
Probably this method should go. It does not really serve a purpose, since what we are checking here is that the build is successful (exitcode 0 should be sufficient)

-thats not entirely correct, we are checking if the build was successful and export of the image is not a tarball for cases where we are not doing a cross platform build.

yeah agreed removing hardcoded stuff, not sure about a good way to test it out, let me check.

apostasie commented 1 week ago
Probably this method should go. It does not really serve a purpose, since what we are checking here is that the build is successful (exitcode 0 should be sufficient)

-thats not entirely correct, we are checking if the build was successful and export of the image is not a tarball for cases where we are not doing a cross platform build.

Besides the fact that currently it is testing stdout instead of stderr - I don't think the above is accurate:

nerdctl build -t foo --platform linux/amd64 . 2>&1 | grep tarball
#5 sending tarball 0.1s done

nerdctl build -t foo --platform linux/arm64 . 2>&1 | grep tarball
#5 sending tarball 0.1s done

nerdctl build -t foo . 2>&1 | grep tarball
#5 sending tarball 0.1s done

Either way, no point in rehashing this - appreciate if you can fix it :-)

Thanks @Shubhranshu153

Shubhranshu153 commented 1 week ago

i see thanks, will update it to test it accurately.

Shubhranshu153 commented 1 week ago

Checked it again, i think we can remove the test creating a pr. Technically the unit test for this cover the different cases for the function itself.

But here is what my test outputs are, so if the platform matches or i dont give platform the tarball is not created, just curious whats your setup to be able to reproduce it, in the grand scheme of things i had thought this behaviour would be consistent in any system

[shubhum@lima-finch fix-shareability-test]$ sudo nerdctl build -t foo --platform linux/amd64 . 2>&1  | grep tarball
[shubhum@lima-finch fix-shareability-test]$ sudo nerdctl build -t foo --platform linux/arm64 . 2>&1  | grep tarball
#5 sending tarball
#5 sending tarball 0.3s done
[shubhum@lima-finch fix-shareability-test]$
[shubhum@lima-finch fix-shareability-test]$
[shubhum@lima-finch fix-shareability-test]$ nerdctl build -t foo . 2>&1 | grep tarball
[shubhum@lima-finch fix-shareability-test]$ nerdctl build -t foo . 2>&1 | grep tarball
[shubhum@lima-finch fix-shareability-test]$ sudo nerdctl build -t foo --platform linux/arm64/v7 . 2>&1  | grep tarball
#4 sending tarball
#4 sending tarball 0.2s done
apostasie commented 1 week ago

But here is what my test outputs are, so if the platform matches or i dont give platform the tarball is not created, just curious whats your setup to be able to reproduce it, in the grand scheme of things i had thought this behaviour would be consistent in any system

I am on lima on an M2 - containerd v2.0.0-rc.3 - nerdctl main.

Not sure what the (other) differences could be here?

Checked it again, i think we can remove the test creating a pr.

That, or we clarify what the intention of the test was. It is fine if we want to keep a test for cross-building, but we just have to make sure:

Shubhranshu153 commented 1 week ago

checked with containerd v2.0.0-rc.4-56-ga44804738 -nerdctl main, my results were same, The intention is simple:

  1. Check that tarballs are not created if we dont do cross arch builds, this saves time.
  2. i can a check if the host checks cross builds but not sure it is adding much value to the unit test as we already make sure the build is successful in other tests.
Shubhranshu153 commented 1 week ago

this is the pr: https://github.com/containerd/nerdctl/pull/3456