Skip to content
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

TestBuildIsShareableForCompatiblePlatform unclear what the intent is #3451

Closed
apostasie opened this issue Sep 21, 2024 · 11 comments
Closed

TestBuildIsShareableForCompatiblePlatform unclear what the intent is #3451

apostasie opened this issue Sep 21, 2024 · 11 comments
Labels
kind/unconfirmed-bug-claim Unconfirmed bug claim

Comments

@apostasie
Copy link
Contributor

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:

  • verify that we can build, which is already in another test
  • verify that we can build on a different platform, which assumes emulation is installed

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 apostasie added the kind/unconfirmed-bug-claim Unconfirmed bug claim label Sep 21, 2024
@apostasie
Copy link
Contributor Author

apostasie commented Sep 21, 2024

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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

@apostasie apostasie mentioned this issue Sep 22, 2024
3 tasks
@Shubhranshu153
Copy link
Contributor

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
Copy link
Contributor Author

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:

  • we skip the test if the host does not have support for cross building
  • we assert success somehow

@Shubhranshu153
Copy link
Contributor

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
Copy link
Contributor

this is the pr: #3456

@apostasie
Copy link
Contributor Author

PR removed this test, so, closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/unconfirmed-bug-claim Unconfirmed bug claim
Projects
None yet
Development

No branches or pull requests

2 participants