intergration-cli: migrate TestContainerAPIRename to cli e2e test by carsontham · Pull Request #6212 · docker/cli

carsontham · 2025-07-25T18:33:02Z

- What I did
Migrated the TestContainerAPIRename to integration tests, reference: moby/moby#50159

- How I did it
renamed a running container and checked the container has been updated to the new name

- How to verify it
run the e2e container tests:
TESTDIRS="./e2e/container/..." TESTFLAGS="-test.run=TestContainerRename" make -f docker.Makefile test-e2e-local

- Human readable description for the release notes

- A picture of a cute animal (not mandatory but encouraged)

thaJeztah

Thank you! As I saw you were working through migrating various tests in moby, I spend a bit extra time on review to leave some suggestions. Sorry for "nit-picking" on some, but I thought I'd leave some suggestions on what I think is the direction we want to head, because "now" may be the right moment to do so, and the extra information may help you for future PRs.

Feedback on my suggestions is more than welcome - let me know if you agree with them, or if you have other suggestions yourself!

thaJeztah · 2025-07-28T08:33:46Z

e2e/container/rename_test.go

+	oldName := "old_name_" + t.Name()
+	result := icmd.RunCommand("docker", "run", "-d", "--name", oldName, fixtures.AlpineImage, "sleep", "60")
+	result.Assert(t, icmd.Success)
+	containerID := strings.TrimSpace(result.Stdout())


Really small nit, and mostly "preference" - not sure how consistent we are in the CLI repository, but in the "moby" repository, I started to standardise on cID (or cid in some places) for container IDs (and, e.g. nwID for networks) .

It's good practice to keep var names shorter the closer they are to where they're used (and less likely to shadow vars or types defined elsewhere). Standardising on these var-names means that we can keep the names short, and a reader of the code still understands (as it's a common name). A bit similar to using t (in tests) and ctx (for contexts).

The (e2e) tests in docker/cli are not as extensive as the number of tests we have in moby, so "now' may be the right time to start looking at that (I'm considering res for result would work ok, but result is probably still fine).

thaJeztah · 2025-07-28T08:43:18Z

e2e/container/rename_test.go

+	result := icmd.RunCommand("docker", "run", "-d", "--name", oldName, fixtures.AlpineImage, "sleep", "60")
+	result.Assert(t, icmd.Success)
+	containerID := strings.TrimSpace(result.Stdout())
+


I don't think we currently have a proper "main" function to cleanup after the test in this repository, so probably good to add a cleanup step after we verified the container was created, and we got its ID;

cID := strings.TrimSpace(res.Stdout()) t.Cleanup(func() { icmd.RunCommand("docker", "container", "rm", "-f", cID).Assert(t, icmd.Success) })

thaJeztah · 2025-07-28T08:45:38Z

e2e/container/rename_test.go

+	renameResult := icmd.RunCommand("docker", "container", "rename", oldName, newName)
+	renameResult.Assert(t, icmd.Success)


Given that we don't need the result of previous steps anymore, it may be OK to just re-use the same var. It slightly reduces noise; i.e.;

result = icmd.RunCommand("docker", "container", "rename", oldName, newName) result.Assert(t, icmd.Success)

Or , if you choose to use res for the results, it would be;

res = icmd.RunCommand("docker", "container", "rename", oldName, newName) res.Assert(t, icmd.Success)

thaJeztah · 2025-07-28T08:48:32Z

e2e/container/rename_test.go

+	renameResult := icmd.RunCommand("docker", "container", "rename", oldName, newName)
+	renameResult.Assert(t, icmd.Success)
+
+	inspectResult := icmd.RunCommand("docker", "inspect", "--format", "{{.Name}}", containerID)


Tiny "nit"; it's better here to use docker container inspect so that we would only ever get a response for a container with the given ID not anything else. The docker inspect command tries to match any "object" (containers, volumes, networks, etc.).

As we're using an ID of the container, it's very unlikely that we would get anything else, but it may still save some time (daemon doesn't have to look through anything else), and it's good practice to do so. (As this was a test written for the integration-cli, it's probably a very old test, and we probably didn't have docker container inspect (only docker inspect) when the test was written).

res = icmd.RunCommand("docker", "inspect", "--format", "{{.Name}}", cID)

thaJeztah · 2025-07-28T08:49:25Z

e2e/container/rename_test.go

+
+	inspectResult := icmd.RunCommand("docker", "inspect", "--format", "{{.Name}}", containerID)
+	inspectResult.Assert(t, icmd.Success)
+	assert.Equal(t, "/"+newName, strings.TrimSpace(inspectResult.Stdout()))


LOL, the / prefix makes me cringe every time. It's there for historic reasons, but we should really discuss that again.. it's really confusing.

(just a random comment 😂 )

thaJeztah · 2025-07-28T08:53:35Z

Oh! Let me copy/paste here what I wrote on my local machine while reviewing, in case you want to use my suggestions;

func TestContainerRename(t *testing.T) {
	oldName := "old_name_" + t.Name()
	res := icmd.RunCommand("docker", "run", "-d", "--name", oldName, fixtures.AlpineImage, "sleep", "60")
	res.Assert(t, icmd.Success)
	cID := strings.TrimSpace(res.Stdout())
	t.Cleanup(func() {
		icmd.RunCommand("docker", "container", "rm", "-f", cID).Assert(t, icmd.Success)
	})

	newName := "new_name_" + t.Name()
	res = icmd.RunCommand("docker", "container", "rename", oldName, newName)
	res.Assert(t, icmd.Success)

	res = icmd.RunCommand("docker", "container", "inspect", "--format", "{{.Name}}", cID)
	res.Assert(t, icmd.Success)
	assert.Equal(t, "/"+newName, strings.TrimSpace(res.Stdout()))
}

carsontham · 2025-07-28T16:17:35Z

hey @thaJeztah! thanks for taking the time to provide such detailed review for this PR. I agree with the suggestions. It's my first few PRs in the project and I wasn't aware of some of the standardisation, will take note of these points 👍😄

i have fixed the test based on the suggestions. thanks again!

codecov-commenter · 2025-07-28T16:50:01Z

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Signed-off-by: carsontham <carsontham@outlook.com>

thaJeztah · 2025-07-29T18:17:14Z

hey @thaJeztah! thanks for taking the time to provide such detailed review for this PR. I agree with the suggestions. It's my first few PRs in the project and I wasn't aware of some of the standardisation, will take note of these points 👍😄

You're welcome! We haven't standardized things everywhere (and they are definitely not "hard rules"). The codebase is over 10 years old, and especially the older often doesn't follow modern standards, and often is inconsistent (because there weren't many good examples yet in the code). At the time, the Go ecosystem wasn't as large, so many things had to be "invented from scratch". And newer tests take advantage of that (sometimes basic things, like using subtests, or using t.TempDir().

So now when we update tests, it's also good to bring them a bit more in the modern world; also to bring more consistency between tests. And, for some tests, it may be that we discover the test is already covered elsewhere (so maybe fully redundant).

I have fixed the test based on the suggestions. thanks again!

Thank you! Changes looked good; I did a quick rebase and squashed the commits; in this case we didn't need to preserve the history, so squashing the commits was cleaner.

thaJeztah

LGTM, thanks!

carsontham force-pushed the e2e-test-container-rename branch from ab6a217 to 90d84f3 Compare July 25, 2025 18:36

carsontham mentioned this pull request Jul 25, 2025

integration-cli: migrated test TestContainerAPIRename to docker/cli moby/moby#50507

Merged

thaJeztah added status/2-code-review area/testing labels Jul 26, 2025

thaJeztah added this to the 29.0.0 milestone Jul 26, 2025

thaJeztah reviewed Jul 28, 2025

View reviewed changes

migrate e2e container rename test

149503a

Signed-off-by: carsontham <carsontham@outlook.com>

thaJeztah force-pushed the e2e-test-container-rename branch from 7f8f117 to 149503a Compare July 29, 2025 18:07

thaJeztah approved these changes Jul 29, 2025

View reviewed changes

thaJeztah merged commit 2199a05 into docker:master Jul 29, 2025
87 checks passed

vvoland mentioned this pull request Aug 4, 2025

integration-cli migration moby/moby#50159

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

intergration-cli: migrate TestContainerAPIRename to cli e2e test#6212

intergration-cli: migrate TestContainerAPIRename to cli e2e test#6212
thaJeztah merged 1 commit intodocker:masterfrom
carsontham:e2e-test-container-rename

carsontham commented Jul 25, 2025

Uh oh!

thaJeztah left a comment

Uh oh!

thaJeztah Jul 28, 2025

Uh oh!

thaJeztah Jul 28, 2025

Uh oh!

thaJeztah Jul 28, 2025

Uh oh!

thaJeztah Jul 28, 2025

Uh oh!

thaJeztah Jul 28, 2025

Uh oh!

carsontham Jul 28, 2025

Uh oh!

thaJeztah commented Jul 28, 2025 •

edited

Loading

Uh oh!

carsontham commented Jul 28, 2025 •

edited

Loading

Uh oh!

codecov-commenter commented Jul 28, 2025

Uh oh!

thaJeztah commented Jul 29, 2025

Uh oh!

thaJeztah left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		renameResult := icmd.RunCommand("docker", "container", "rename", oldName, newName)
		renameResult.Assert(t, icmd.Success)

Conversation

carsontham commented Jul 25, 2025

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah Jul 28, 2025

Choose a reason for hiding this comment

Uh oh!

thaJeztah Jul 28, 2025

Choose a reason for hiding this comment

Uh oh!

thaJeztah Jul 28, 2025

Choose a reason for hiding this comment

Uh oh!

thaJeztah Jul 28, 2025

Choose a reason for hiding this comment

Uh oh!

thaJeztah Jul 28, 2025

Choose a reason for hiding this comment

Uh oh!

carsontham Jul 28, 2025

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Jul 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carsontham commented Jul 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Jul 28, 2025

Codecov Report

Uh oh!

thaJeztah commented Jul 29, 2025

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

thaJeztah commented Jul 28, 2025 •

edited

Loading

carsontham commented Jul 28, 2025 •

edited

Loading