Test rework, part 4 #3492

apostasie · 2024-10-03T22:01:21Z

This is the next part of the test rewriting project.

Background and motivation:

See:

and the initial commit message from 2c95d09
along with the linked issues from above PRs.

The gist of it is really simple: our testing situation is not good (see prior discussions for full list of issues - but then, the sheer fact we have to retry our CI multiple times to get a green tree should say enough).

This here is the part 4 of the effort to rewrite our tests using a new, custom developed testing toolkit that emphasizes test expressiveness, debugability and better isolation.

Changes

Individual commits messages have extensive details about the changes where appropriate.

Status

a. the following has been ran locally 100x+ with no failure:

. (< 1 sec)
completion (< 1 sec)
issues (< 10 secs)
system (~ 5 secs)
volume (~ 5 secs)
network (~ 5 secs)
- although, network create fail: route ip+net: no such network interface #3500 did show up and might still be lurking (~~and network inspect fail: conflist: no such file or directory (returns...) #3501~~, albeit this one probably got fixed by Fix raft of issues related to network listing #3508)

b. the following have ran locally 20x+ with no failure:

ipfs (~ 100 secs)

c. the following are failing after about 20 iterations:

image (~40 secs)
- likely due to one of the variants in content digest not found #3513 (the one with images --filter or the one using a platform reduced push)

d. the following are NOT part of this PR:

builder
container
compose
login

Also left out of this PR: disabling retries for non flaky tests.
We do now have the infrastructure for that (eg: -test.only-flaky) but this will come in another PR after this.

Note that while no-failure locally with 100+ runs is no guarantee that things are fine: I do run local tests directly on the host, not inside the dockerfile - running inside the dockerfile on the CI is likely to bring up a lot more failures, either because things are inherently slower, or because the extra docker layer is adding specific conditions that will make us fail.

Review

Close to 10k lines is indeed a chunky PR, but then, as with the previous parts, there is absolutely no "code" or "logic" changes in nerdctl itself - this is "just" tests: if they run, and if the CI is green...

PR has been split in as many commits as feasible to facilitate review, along with a number of inline comments in the github UI where appropriate.

Let me know if I can make this easier in any way.

The north star for this whole effort remains:

we must get to a consistently green CI without retries - with no or very few flaky test.
go test -p 1 ./cmd/nerdctl/... must work locally out of the box on developers laptop without randomly breaking for missing requirements

apostasie · 2024-10-05T08:19:27Z

cmd/nerdctl/volume/volume_create_test.go

-			Command:     test.RunCommand("volume", "create"),
-			Expected:    test.Expects(0, nil, nil),
+			Command:     test.Command("volume", "create"),
+			Expected:    test.Expects(0, nil, test.Match(regexp.MustCompile("^[a-f0-9]{64}\n$"))),


Enhance the test a little bit.

apostasie · 2024-10-05T08:19:56Z

cmd/nerdctl/volume/volume_create_test.go

-					Errors:   []error{errdefs.ErrInvalidArgument},
-				}
-			},
+			// NOTE: docker returns 125 on this


Use compact syntax.

apostasie · 2024-10-05T08:21:20Z

cmd/nerdctl/volume/volume_inspect_test.go

-			assert.NilError(t, err, "File creation failed")
+	testCase := nerdtest.Setup()
+
+	testCase.Require = nerdtest.BrokenTest("This test assumes that the host-side of a volume can be written into, "+


Marking this test as broken (for Docker without root)

apostasie · 2024-10-05T08:22:32Z

cmd/nerdctl/volume/volume_list_test.go

-		Setup: func(data test.Data, helpers test.Helpers) {
-			var vol1, vol2, vol3, vol4 = data.Identifier() + "-1", data.Identifier() + "-2", data.Identifier() + "-3", data.Identifier() + "-4"
-			var label1, label2, label3, label4 = data.Identifier() + "=label-1", data.Identifier() + "=label-2", data.Identifier() + "=label-3", data.Identifier() + "-group=label-4"
+	testCase.Require = nerdtest.BrokenTest("This test assumes that the host-side of a volume can be written into, "+


Marking test as broken.

apostasie · 2024-10-05T08:27:07Z

cmd/nerdctl/main_test_test.go

 					},
 					Expected: test.Expects(0, nil, test.Equals("uninitialized-setup-command")),
 				},
 			},
 			Expected: test.Expects(0, nil, test.Equals("uninitialized-setup")),
 		},
-		{


WithEnv is no longer exposed. This tests are now moot.

apostasie · 2024-10-05T08:27:45Z

cmd/nerdctl/main_test.go

-				Description: "TOML > Default",
-				Command:     test.RunCommand("info", "-f", "{{.Driver}}"),
-				Expected:    test.Expects(0, nil, test.Equals("dummy-snapshotter-via-toml\n")),
-				Data:        test.WithConfig(nerdtest.NerdctlToml, `snapshotter = "dummy-snapshotter-via-toml"`),


Config is now separate from Data.

apostasie · 2024-10-05T08:28:36Z

cmd/nerdctl/issues/main_linux_test.go

-				return helpers.
-					Command("run", "-it", "--rm", "--net=host", testutil.AlpineImage, "echo", "this was always working").
-					WithWrapper("unbuffer")
+			Command: func(data test.Data, helpers test.Helpers) test.TestableCommand {


Chaining was a mistake.

apostasie · 2024-10-05T08:29:01Z

cmd/nerdctl/issues/main_linux_test.go

@@ -14,7 +14,7 @@
   limitations under the License.
 */

-package main
+package issues


Moving this file out of the root, where it does not belong.

apostasie · 2024-10-05T08:29:50Z

cmd/nerdctl/issues/issues_linux_test.go

 func TestIssue3425(t *testing.T) {
 	nerdtest.Setup()

 	var registry *testregistry.RegistryServer

-	var ipfsPath string


Now embedded inside the Requirement IPFS

apostasie · 2024-10-05T08:30:08Z

cmd/nerdctl/issues/issues_linux_test.go

 				),
-				Env: map[string]string{


nerdtest.IPFS requirement now takes care of this.

apostasie · 2024-10-10T21:20:21Z

There is clearly a deadlock happening in image tests, but only for arm64.

Debugging right now - marking draft until I find the culprit.

Signed-off-by: apostasie <[email protected]>

This commit bundles a few minor fixes that are necessary following changes in test tooling. Signed-off-by: apostasie <[email protected]>

apostasie · 2024-10-11T00:12:19Z

Fixed.

apostasie · 2024-10-11T18:27:35Z

cmd/nerdctl/volume/volume_inspect_test.go

@@ -51,7 +51,7 @@ func TestVolumeInspect(t *testing.T) {
 		&test.Requirement{
 			Check: func(data test.Data, helpers test.Helpers) (bool, string) {
 				isDocker, _ := nerdtest.Docker.Check(data, helpers)
-				return !isDocker || test.IsRoot(), "docker cli needs to be run as root"
+				return !isDocker || os.Geteuid() == 0, "docker cli needs to be run as root"


Based on reviewers feedback.

apostasie · 2024-10-11T18:27:41Z

cmd/nerdctl/volume/volume_list_test.go

@@ -94,7 +95,7 @@ func TestVolumeLsFilter(t *testing.T) {
 		&test.Requirement{
 			Check: func(data test.Data, helpers test.Helpers) (bool, string) {
 				isDocker, _ := nerdtest.Docker.Check(data, helpers)
-				return !isDocker || test.IsRoot(), "docker cli needs to be run as root"
+				return !isDocker || os.Geteuid() == 0, "docker cli needs to be run as root"


Based on reviewers feedback.

apostasie · 2024-10-11T18:27:47Z

pkg/testutil/nerdtest/requirements.go

@@ -32,7 +32,7 @@ import (
 	"github.com/containerd/nerdctl/v2/pkg/testutil/test"
 )

-var BuildkitHost test.ConfigKey = "bkHost"
+var BuildkitHost test.ConfigKey = "BuildkitHost"


Based on reviewers feedback.

apostasie · 2024-10-11T18:27:52Z

pkg/testutil/test/utilities.go

 )

-// IsRoot returns true if we are root... simple


Based on reviewers feedback.

apostasie · 2024-10-11T18:27:58Z

pkg/testutil/testutil.go

@@ -562,7 +562,7 @@ func M(m *testing.M) {
 	flag.BoolVar(&flagTestKillDaemon, "test.allow-kill-daemon", false, "enable tests that kill the daemon")
 	flag.BoolVar(&flagTestIPv6, "test.only-ipv6", false, "enable tests on IPv6")
 	flag.BoolVar(&flagTestKube, "test.only-kubernetes", false, "enable tests on Kubernetes")
-	flag.BoolVar(&flagTestFlaky, "test.only-flaky", false, "enable testing of flaky tests only")


Based on reviewers feedback.

AkihiroSuda · 2024-10-13T18:58:07Z

pkg/testutil/nerdtest/ambient.go

+
+func environmentIsForFlaky() bool {
+	return testutil.GetFlakyEnvironment()
+}


Why not just call testutil functions directly

Currently we have to bridge between the legacy tests, and the code handling flag parsing, and the proposed new framework, which unfortunately implies a certain level of duplication.
I will clean that up once all tests are migrated.

AkihiroSuda · 2024-10-13T18:59:42Z

pkg/testutil/nerdtest/platform/platform_linux.go

+	RegistryImageNext   = testutil.RegistryImageNext
+	KuboImage           = testutil.KuboImage
+	DockerAuthImage     = testutil.DockerAuthImage
+)


Why can't just use testutil constants

Because we are stuck in a bad situation right now with a mix of _platform_test.go files (which should disappear) and Skip(platform) tests, that must have the corresponding helpers available.

This will get cleaned-up later on, but rn a level of indirection is necessary (or alternatively add a bunch of stuff in the legacy files).

AkihiroSuda · 2024-10-13T19:01:33Z

pkg/testutil/nerdtest/requirements.go

+}
+
+// RootLess marks a test as suitable only for the rootless environment
+var RootLess = &test.Requirement{


maybe s/RootLess/Rootless/

Yeah, I was wondering about that...

I am happy either way - I will take another look at that aspect (RootLess, RootFul) with the next round then.

AkihiroSuda · 2024-10-13T19:02:58Z

pkg/testutil/nerdtest/utilities.go

 	"github.com/containerd/nerdctl/v2/pkg/testutil/test"
 )

+func IsDocker() bool {
+	return testutil.GetTarget() == "docker"
+}


Why not just call testutil.GetTarget directly

Migrating to the proposed new framework, I would rather have everything sourced from there to help cleaning things up.
And yes, eventually, they will be merged.

AkihiroSuda

Thanks, I'm merging this, but this amount of PR was almost unreviewable and even causing Safari to almost hang up 😓

In the next series of the PR please consider splitting the PR to smaller parts.

Also consider reducing the amount of the helper functions and constants (see the comments)

apostasie · 2024-10-13T23:53:02Z

Thanks, I'm merging this, but this amount of PR was almost unreviewable and even causing Safari to almost hang up 😓

@AkihiroSuda I really appreciate your help and support on this.
I could not have done it otherwise.

In the next series of the PR please consider splitting the PR to smaller parts.

Upcoming parts are roughly:

github actions clean-up with non-flaky/flaky(with retry) split up - this will give us a lot of bang for the buck and likely a good speed-up of the total testrun time
migration of container tests
migration of builder tests
migration of login tests
migration of compose tests

I think the test framework is mature enough now that we can have a separate PR for each of these ^ - making it much less painful for everybody (me included 😓).

Also consider reducing the amount of the helper functions and constants (see the comments)

Eventually all of it will fuse into one - rn I have to make sure existing tests still work unaltered, and the new ones are not constrained... It is tough. I almost gave up on the effort multiple times 😰. Debugging some of these issues was mind altering.

Thanks again!
I am absolutely convinced we will be in a much better situation soon, and the quality of 2.0 will rock!

apostasie force-pushed the IGNORE branch 5 times, most recently from 8946f8f to fe54f32 Compare October 5, 2024 07:42

apostasie changed the title ~~[IGNORE] CI testing~~ [WIP] Test rework, part 4 Oct 5, 2024

apostasie force-pushed the IGNORE branch from fe54f32 to 0146f28 Compare October 5, 2024 08:06

apostasie commented Oct 5, 2024

View reviewed changes

cmd/nerdctl/issues/issues_linux_test.go

),

Env: map[string]string{

Copy link

Contributor Author

apostasie Oct 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nerdtest.IPFS requirement now takes care of this.

apostasie force-pushed the IGNORE branch 6 times, most recently from a374cf1 to f7cfb10 Compare October 6, 2024 02:43

apostasie mentioned this pull request Oct 7, 2024

Fix raft of issues related to network listing #3508

Merged

apostasie force-pushed the IGNORE branch 6 times, most recently from c00c943 to d0ca83a Compare October 7, 2024 06:40

apostasie force-pushed the IGNORE branch 2 times, most recently from f772e84 to a7bb358 Compare October 10, 2024 21:19

apostasie marked this pull request as draft October 10, 2024 21:20

apostasie force-pushed the IGNORE branch 6 times, most recently from 498bdfb to a71ecf3 Compare October 10, 2024 22:42

apostasie added 2 commits October 10, 2024 16:33

Migrate image tests

4451d75

Signed-off-by: apostasie <[email protected]>

Migration aftermath

33beb32

This commit bundles a few minor fixes that are necessary following changes in test tooling. Signed-off-by: apostasie <[email protected]>

apostasie force-pushed the IGNORE branch from a71ecf3 to 33beb32 Compare October 10, 2024 23:34

apostasie marked this pull request as ready for review October 10, 2024 23:35

apostasie mentioned this pull request Oct 11, 2024

Rewrite cp #3323

Merged

AkihiroSuda requested a review from a team October 11, 2024 08:59

apostasie commented Oct 11, 2024

View reviewed changes

pkg/testutil/test/utilities.go

)

// IsRoot returns true if we are root... simple

Copy link

Contributor Author

apostasie Oct 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on reviewers feedback.

apostasie commented Oct 11, 2024

View reviewed changes

AkihiroSuda reviewed Oct 13, 2024

View reviewed changes

AkihiroSuda approved these changes Oct 13, 2024

View reviewed changes

AkihiroSuda merged commit 6abe279 into containerd:main Oct 13, 2024
22 checks passed

AkihiroSuda added the area/ci e.g., CI failure label Oct 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test rework, part 4 #3492

Test rework, part 4 #3492

apostasie commented Oct 3, 2024 •

edited

Loading

apostasie Oct 5, 2024

apostasie Oct 5, 2024 •

edited

Loading

apostasie Oct 5, 2024

apostasie Oct 5, 2024

apostasie Oct 5, 2024

apostasie Oct 5, 2024

apostasie Oct 5, 2024

apostasie Oct 5, 2024

apostasie Oct 5, 2024

apostasie Oct 5, 2024

apostasie commented Oct 10, 2024

apostasie commented Oct 11, 2024

apostasie Oct 11, 2024

apostasie Oct 11, 2024

apostasie Oct 11, 2024

apostasie Oct 11, 2024

apostasie Oct 11, 2024

AkihiroSuda Oct 13, 2024

apostasie Oct 13, 2024

AkihiroSuda Oct 13, 2024

apostasie Oct 13, 2024

AkihiroSuda Oct 13, 2024 •

edited

Loading

apostasie Oct 13, 2024

AkihiroSuda Oct 13, 2024

apostasie Oct 13, 2024

AkihiroSuda left a comment

apostasie commented Oct 13, 2024

Test rework, part 4 #3492

Test rework, part 4 #3492

Conversation

apostasie commented Oct 3, 2024 • edited Loading

Background and motivation:

Changes

Status

Review

Choose a reason for hiding this comment

apostasie Oct 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apostasie commented Oct 10, 2024

apostasie commented Oct 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AkihiroSuda Oct 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AkihiroSuda left a comment

Choose a reason for hiding this comment

apostasie commented Oct 13, 2024

apostasie commented Oct 3, 2024 •

edited

Loading

apostasie Oct 5, 2024 •

edited

Loading

AkihiroSuda Oct 13, 2024 •

edited

Loading