migrate TestAPINetworkConnectDisconnect to integration test by 2003Aditya · Pull Request #51568 · moby/moby

2003Aditya · 2025-11-20T11:52:21Z

- What I did
Migrated the TestAPINetworkConnectDisconnect test from integration-cli/docker_api_network_test.go to the new integration test framework under integration/network/network_test.go.

robmry · 2025-11-26T11:35:21Z

integration/network/network_test.go

+	createNet, err := apiClient.NetworkCreate(ctx, name, client.NetworkCreateOptions{})
+	assert.NilError(t, err)
+
+	nr, err := apiClient.NetworkInspect(ctx, createNet.ID, client.NetworkInspectOptions{})
+	assert.NilError(t, err)
+
+	assert.Equal(t, nr.Network.Name, name)
+	assert.Equal(t, nr.Network.ID, createNet.ID)
+	assert.Equal(t, len(nr.Network.Containers), 0)
+
+	t.Cleanup(func() {
+		_, _ = apiClient.NetworkRemove(ctx, nr.Network.ID, client.NetworkRemoveOptions{})
+	})


We've, surprisingly, got a lot of test client utils that make the tests shorter ... there are examples in bridge_linux_test.go and others.

Putting the cleanup after the assertions that follow the NetworkCreate mean the network won't be deleted if any of those assertions fails.

So, this would be ...

network.CreateNoError(ctx, t, apiClient, name) defer network.RemoveNoError(ctx, t, apiClient, name) <the inspect code>

robmry · 2025-11-26T12:04:13Z

integration/network/network_test.go

+	createResp, err := apiClient.ContainerCreate(ctx, client.ContainerCreateOptions{
+		Name: containerName,
+		Config: &container.Config{
+			Image: "busybox",
+			Cmd:   []string{"top"},
+		},
+	})
+	assert.NilError(t, err)
+
+	t.Cleanup(func() {
+		_, _ = apiClient.ContainerRemove(ctx, createResp.ID, client.ContainerRemoveOptions{})
+	})
+
+	_, err = apiClient.ContainerStart(ctx, createResp.ID, client.ContainerStartOptions{})
+	assert.NilError(t, err)


This can be ...

id := container.Run(ctx, t, apiClient, container.WithName(containerName)) defer func() { _, _ = apiClient.ContainerRemove(ctx, id, client.ContainerRemoveOptions{}) })

The test client defaults to busybox/top if no image/command is specified.

robmry · 2025-11-26T12:08:08Z

integration/network/network_test.go

 	assert.Assert(t, found, fmt.Sprintf("%s is not found", networkName))
 }
+
+func TestAPINetworkConnectDisconnect(t *testing.T) {


We have other tests that connect/disconnect networks - but I think this one is interesting because it creates a container connected to the default bridge, then connects a user-defined network.

We're probably light on coverage for that, and likely have bugs in the area, because it's not allowed in a create/run command ...

$ docker run --rm -ti --network bridge --network n1 alpine docker: conflicting options: cannot attach both user-defined and non-user-defined network-modes

It'd be worth adding a comment to say that's what it's for. Maybe rename the test to TestBridgeAndCustomNetworkAttach?

robmry · 2025-11-26T12:12:36Z

Oh, for the lint failures - the integration-cli functions can be removed if they're unused ...

integration-cli/docker_api_network_test.go:198:6: func connectNetwork is unused (unused)
func connectNetwork(t *testing.T, nid, cid string) {
     ^
integration-cli/docker_api_network_test.go:206:6: func disconnectNetwork is unused (unused)
func disconnectNetwork(t *testing.T, nid, cid string) {
     ^

robmry

Thank you @2003Aditya.

A couple of minor nits below - but it's quite hard to see at a glance what the test is doing. I think it'd be worth adding comments describing each of the steps (similar to the original test).

integration/network/network_test.go

robmry · 2025-12-05T16:36:25Z

integration/network/network_test.go

+	nr, err = apiClient.NetworkInspect(ctx, name, client.NetworkInspectOptions{})
+	assert.NilError(t, err)
+	assert.Equal(t, len(nr.Network.Containers), 0)
+


nit: blank line

Signed-off-by: Aditya Mishra <mishraaditya675@gmail.com>

robmry

Thank you - some more comments, to align with conventions we try to stick to in the tests ...

robmry · 2025-12-19T15:18:53Z

integration/network/network_test.go

+	assert.Equal(t, nr.Network.Name, name)
+	assert.Equal(t, len(nr.Network.Containers), 0)
+
+	// starting a container without attaching to the network created above


It'd be worth noting this connects the container to the default bridge network - as that's really the value in this test (to have a container connected to both the default bridge and a user-defined network).

Suggested change

// starting a container without attaching to the network created above

// Start a container on the default bridge network.

robmry · 2025-12-19T15:21:38Z

integration/network/network_test.go

+		_, _ = apiClient.ContainerRemove(ctx, id, client.ContainerRemoveOptions{})
+	}()
+
+	// attaching the custom network to the above running conainer .


Typo conainer ...

Suggested change

// attaching the custom network to the above running conainer .

// Attach the container to a custom bridge network.

robmry · 2025-12-19T15:24:15Z

integration/network/network_test.go

+	assert.NilError(t, err)
+	assert.Equal(t, nr.Network.Name, name)
+	assert.Equal(t, len(nr.Network.Containers), 0)


It's worth thinking about whether each assertion needs to stop the test here and in the rest of this test (and in-general).

In this case, if the inspect fails, things are probably so broken it's not worth continuing.

But, in the unlikely event there's a container attached to the new network - might as well let the test carry on to see what else happens.

Suggested change

assert.NilError(t, err)

assert.Equal(t, nr.Network.Name, name)

assert.Equal(t, len(nr.Network.Containers), 0)

assert.NilError(t, err)

assert.Check(t, is.Equal(nr.Network.Name, name))

assert.Check(t, is.Equal(len(nr.Network.Containers), 0))

robmry · 2025-12-19T15:38:33Z

integration/network/network_test.go

+	var containerIP string
+	for n, settings := range contInspect.Container.NetworkSettings.Networks {
+		if n == name {
+			containerIP = settings.IPAddress.String()
+			break
+		}
+	}
+	assert.Assert(t, containerIP != "")
+	assert.Equal(t, nr.Network.Containers[id].IPv4Address.Addr().String(), containerIP)


Networks is a map, so there's no need to loop - and the addresses have type netip.Addr, it's best not to convert to strings unless necessary ...

Suggested change

var containerIP string

for n, settings := range contInspect.Container.NetworkSettings.Networks {

if n == name {

containerIP = settings.IPAddress.String()

break

}

}

assert.Assert(t, containerIP != "")

assert.Equal(t, nr.Network.Containers[id].IPv4Address.Addr().String(), containerIP)

n, ok := contInspect.Container.NetworkSettings.Networks[name]

if assert.Check(t, ok) {

assert.Check(t, n.IPAddress.IsValid())

assert.Check(t, is.Equal(nr.Network.Containers[id].IPv4Address.Addr(), n.IPAddress))

}

robmry · 2025-12-19T15:40:03Z

integration/network/network_test.go

+	// starting a container without attaching to the network created above
+	id := container.Run(ctx, t, apiClient, container.WithName(containerName))
+	defer func() {
+		_, _ = apiClient.ContainerRemove(ctx, id, client.ContainerRemoveOptions{})


We usually force removal for cleanup, to make sure it happens ...

Suggested change

_, _ = apiClient.ContainerRemove(ctx, id, client.ContainerRemoveOptions{})

_, _ = apiClient.ContainerRemove(ctx, id, client.ContainerRemoveOptions{Force: true})

github-actions bot added area/networking Networking area/testing labels Nov 20, 2025

2003Aditya force-pushed the TestAPINetworkConnectDisconnect branch from 3082e80 to b49de7f Compare November 20, 2025 19:28

robmry reviewed Nov 26, 2025

View reviewed changes

2003Aditya force-pushed the TestAPINetworkConnectDisconnect branch 3 times, most recently from 53eaff5 to 171ab7c Compare December 2, 2025 17:05

robmry mentioned this pull request Dec 5, 2025

integration-cli migration #50159

Open

robmry reviewed Dec 5, 2025

View reviewed changes

2003Aditya force-pushed the TestAPINetworkConnectDisconnect branch 3 times, most recently from 954275e to 98d8e60 Compare December 16, 2025 17:27

migrate TestAPINetworkConnectDisconnect to integration test

eae4ec4

Signed-off-by: Aditya Mishra <mishraaditya675@gmail.com>

2003Aditya force-pushed the TestAPINetworkConnectDisconnect branch from 98d8e60 to eae4ec4 Compare December 16, 2025 17:44

robmry reviewed Dec 19, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

migrate TestAPINetworkConnectDisconnect to integration test#51568

migrate TestAPINetworkConnectDisconnect to integration test#51568
2003Aditya wants to merge 1 commit intomoby:masterfrom
2003Aditya:TestAPINetworkConnectDisconnect

2003Aditya commented Nov 20, 2025 •

edited by robmry

Loading

Uh oh!

robmry Nov 26, 2025

Uh oh!

robmry Nov 26, 2025

Uh oh!

robmry Nov 26, 2025

Uh oh!

robmry commented Nov 26, 2025

Uh oh!

robmry left a comment

Uh oh!

Uh oh!

robmry Dec 5, 2025

Uh oh!

robmry left a comment

Uh oh!

robmry Dec 19, 2025

Uh oh!

robmry Dec 19, 2025

Uh oh!

robmry Dec 19, 2025

Uh oh!

robmry Dec 19, 2025

Uh oh!

robmry Dec 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	// starting a container without attaching to the network created above
	// Start a container on the default bridge network.

	// attaching the custom network to the above running conainer .
	// Attach the container to a custom bridge network.

	_, _ = apiClient.ContainerRemove(ctx, id, client.ContainerRemoveOptions{})
	_, _ = apiClient.ContainerRemove(ctx, id, client.ContainerRemoveOptions{Force: true})

Conversation

2003Aditya commented Nov 20, 2025 • edited by robmry Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robmry commented Nov 26, 2025

Uh oh!

robmry left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robmry left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

2003Aditya commented Nov 20, 2025 •

edited by robmry

Loading