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.

Sign up for GitHub

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

Chore: spurious helpers test cleanup #3729

Merged
merged 1 commit into from
Dec 6, 2024
Merged

Chore: spurious helpers test cleanup #3729

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions cmd/nerdctl/image/image_list_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,15 @@ package image
import (
"errors"
"fmt"
"os"
"path/filepath"
"runtime"
"slices"
"strings"
"testing"

"gotest.tools/v3/assert"

testhelpers "github.com/containerd/nerdctl/v2/cmd/nerdctl/helpers"
"github.com/containerd/nerdctl/v2/pkg/tabutil"
"github.com/containerd/nerdctl/v2/pkg/testutil"
"github.com/containerd/nerdctl/v2/pkg/testutil/nerdtest"
Expand Down Expand Up @@ -144,7 +145,9 @@ LABEL foo=bar
LABEL version=0.1
RUN echo "actually creating a layer so that docker sets the createdAt time"
`, testutil.CommonImage)
buildCtx := testhelpers.CreateBuildContext(t, dockerfile)
buildCtx := data.TempDir()
err := os.WriteFile(filepath.Join(buildCtx, "Dockerfile"), []byte(dockerfile), 0o600)
assert.NilError(helpers.T(), err)
Comment on lines +148 to +150
Copy link
Member

Choose a reason for hiding this comment

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

why we cannot use the helper directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The helper is misplaced under /cmd (like a bunch of others that should also be moved to pkg/testing or removed). It is calling t.TempDir() every time it is being called, creating a new separate temporary directory at each invocation, and buries trivial logic away from the test - for the benefit of replacing 3 lines for 1.

I do not think it is worth it. If folks here prefer to keep a helper, we should copy it over to pkg/test and change the signature of the function to at least allow passing data and helpers so that is uses data.TempDir and helpers.T instead.

Copy link
Contributor Author

@apostasie apostasie Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

(I say "copy it over" because a bunch of legacy tests are still dependent on the existing helper and would not be able to use the new one until rewritten)

data.Set("buildCtx", buildCtx)
},
Cleanup: func(data test.Data, helpers test.Helpers) {
Expand Down Expand Up @@ -290,7 +293,9 @@ func TestImagesFilterDangling(t *testing.T) {
dockerfile := fmt.Sprintf(`FROM %s
CMD ["echo", "nerdctl-build-notag-string"]
`, testutil.CommonImage)
buildCtx := testhelpers.CreateBuildContext(t, dockerfile)
buildCtx := data.TempDir()
err := os.WriteFile(filepath.Join(buildCtx, "Dockerfile"), []byte(dockerfile), 0o600)
assert.NilError(helpers.T(), err)
data.Set("buildCtx", buildCtx)
},
Cleanup: func(data test.Data, helpers test.Helpers) {
Expand Down
19 changes: 14 additions & 5 deletions cmd/nerdctl/image/image_prune_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,14 @@ package image

import (
"fmt"
"os"
"path/filepath"
"strings"
"testing"
"time"

"gotest.tools/v3/assert"

testhelpers "github.com/containerd/nerdctl/v2/cmd/nerdctl/helpers"
"github.com/containerd/nerdctl/v2/pkg/testutil"
"github.com/containerd/nerdctl/v2/pkg/testutil/nerdtest"
"github.com/containerd/nerdctl/v2/pkg/testutil/test"
Expand Down Expand Up @@ -61,7 +62,9 @@ func TestImagePrune(t *testing.T) {
CMD ["echo", "nerdctl-test-image-prune"]
`, testutil.CommonImage)

buildCtx := testhelpers.CreateBuildContext(t, dockerfile)
buildCtx := data.TempDir()
err := os.WriteFile(filepath.Join(buildCtx, "Dockerfile"), []byte(dockerfile), 0o600)
assert.NilError(helpers.T(), err)
helpers.Ensure("build", buildCtx)
// After we rebuild with tag, docker will no longer show the <none> version from above
// Swapping order does not change anything.
Expand Down Expand Up @@ -107,7 +110,9 @@ func TestImagePrune(t *testing.T) {
CMD ["echo", "nerdctl-test-image-prune"]
`, testutil.CommonImage)

buildCtx := testhelpers.CreateBuildContext(t, dockerfile)
buildCtx := data.TempDir()
err := os.WriteFile(filepath.Join(buildCtx, "Dockerfile"), []byte(dockerfile), 0o600)
assert.NilError(helpers.T(), err)
helpers.Ensure("build", buildCtx)
helpers.Ensure("build", "-t", identifier, buildCtx)
imgList := helpers.Capture("images")
Expand Down Expand Up @@ -149,7 +154,9 @@ func TestImagePrune(t *testing.T) {
CMD ["echo", "nerdctl-test-image-prune-filter-label"]
LABEL foo=bar
LABEL version=0.1`, testutil.CommonImage)
buildCtx := testhelpers.CreateBuildContext(t, dockerfile)
buildCtx := data.TempDir()
err := os.WriteFile(filepath.Join(buildCtx, "Dockerfile"), []byte(dockerfile), 0o600)
assert.NilError(helpers.T(), err)
helpers.Ensure("build", "-t", data.Identifier(), buildCtx)
imgList := helpers.Capture("images")
assert.Assert(t, strings.Contains(imgList, data.Identifier()), "Missing "+data.Identifier())
Expand Down Expand Up @@ -187,7 +194,9 @@ LABEL version=0.1`, testutil.CommonImage)
dockerfile := fmt.Sprintf(`FROM %s
RUN echo "Anything, so that we create actual content for docker to set the current time for CreatedAt"
CMD ["echo", "nerdctl-test-image-prune-until"]`, testutil.CommonImage)
buildCtx := testhelpers.CreateBuildContext(t, dockerfile)
buildCtx := data.TempDir()
err := os.WriteFile(filepath.Join(buildCtx, "Dockerfile"), []byte(dockerfile), 0o600)
assert.NilError(helpers.T(), err)
helpers.Ensure("build", "-t", data.Identifier(), buildCtx)
imgList := helpers.Capture("images")
assert.Assert(t, strings.Contains(imgList, data.Identifier()), "Missing "+data.Identifier())
Expand Down
10 changes: 8 additions & 2 deletions cmd/nerdctl/image/image_pull_linux_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ package image

import (
"fmt"
"os"
"path/filepath"
"strconv"
"strings"
"testing"
Expand Down Expand Up @@ -56,7 +58,9 @@ func TestImagePullWithCosign(t *testing.T) {
CMD ["echo", "nerdctl-build-test-string"]
`, testutil.CommonImage)

buildCtx := testhelpers.CreateBuildContext(t, dockerfile)
buildCtx := data.TempDir()
err := os.WriteFile(filepath.Join(buildCtx, "Dockerfile"), []byte(dockerfile), 0o600)
assert.NilError(helpers.T(), err)
helpers.Ensure("build", "-t", testImageRef+":one", buildCtx)
helpers.Ensure("build", "-t", testImageRef+":two", buildCtx)
helpers.Ensure("push", "--sign=cosign", "--cosign-key="+keyPair.PrivateKey, testImageRef+":one")
Expand Down Expand Up @@ -120,7 +124,9 @@ func TestImagePullPlainHttpWithDefaultPort(t *testing.T) {
CMD ["echo", "nerdctl-build-test-string"]
`, testutil.CommonImage)

buildCtx := testhelpers.CreateBuildContext(t, dockerfile)
buildCtx := data.TempDir()
err := os.WriteFile(filepath.Join(buildCtx, "Dockerfile"), []byte(dockerfile), 0o600)
assert.NilError(helpers.T(), err)
helpers.Ensure("build", "-t", testImageRef, buildCtx)
helpers.Ensure("--insecure-registry", "push", testImageRef)
helpers.Ensure("rmi", "-f", testImageRef)
Expand Down
7 changes: 5 additions & 2 deletions cmd/nerdctl/ipfs/ipfs_registry_linux_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,15 @@ package ipfs

import (
"fmt"
"os"
"path/filepath"
"regexp"
"strings"
"testing"
"time"

"gotest.tools/v3/assert"

testhelpers "github.com/containerd/nerdctl/v2/cmd/nerdctl/helpers"
"github.com/containerd/nerdctl/v2/pkg/testutil"
"github.com/containerd/nerdctl/v2/pkg/testutil/nerdtest"
"github.com/containerd/nerdctl/v2/pkg/testutil/test"
Expand Down Expand Up @@ -132,7 +133,9 @@ func TestIPFSNerdctlRegistry(t *testing.T) {
CMD ["echo", "nerdctl-build-test-string"]
`, data.Get(ipfsImageURLKey))

buildCtx := testhelpers.CreateBuildContext(t, dockerfile)
buildCtx := data.TempDir()
err := os.WriteFile(filepath.Join(buildCtx, "Dockerfile"), []byte(dockerfile), 0o600)
assert.NilError(helpers.T(), err)

helpers.Ensure("build", "-t", data.Identifier("built-image"), buildCtx)
},
Expand Down