Split Linux/bridge and Windows/nat integration tests#48247
Split Linux/bridge and Windows/nat integration tests#48247akerouanton merged 1 commit intomoby:masterfrom
Conversation
robmry
added
area/networking
| // TestBridgeINC makes sure two containers on two different bridge networks can't communicate with each other. | ||
| func TestBridgeINC(t *testing.T) { | ||
| skip.If(t, testEnv.DaemonInfo.OSType == "windows") |
There was a problem hiding this comment.
So, I know various tests were not put in platform specific files, but skipped because they were not necessary platform-specific, just not yet rewritten to make them work on Windows; from the description of the test, it seems like testing that networking being properly sandboxed should be something that's also the case on Windows; so we know why we had to skip?
Or will there be a new, equivalent test in the Windows file?
There was a problem hiding this comment.
I think that's the only test that should currently have a Windows equivalent but doesn't. We'll need to add more when we get IPv6 on Windows ...
- Should be tested on both ...
TestBridgeINC
- IPv6 tests that we'll need on both when we have IPv6 support on Windows (depending on what we get working)...
TestDefaultBridgeIPv6TestDefaultBridgeAddressesTestDisableIPv6AddrsTestNonIPv6NetworkTestNoIP6TablesTestDisableNATTestProxy4To6
- Linux-specific ...
TestInternalNwConnectivity(--internal doesn't work on Windows)TestSetInterfaceSysctlTestReadOnlySlashProcTestSetEndpointSysctl
- Tested on both ...
TestBridgeICCTestPortMappedHairpin
I haven't discovered a way to run integration tests locally on Windows, so writing them is a bit of a nightmare. I don't want to get into it as part of this change - so I've raised #48249.
Most tests in integration/networking/bridge_test.go are skipped on Windows - and I want to add a test that uses helpers that aren't implemented on Windows. So, move it to bridge_linux_test.go, remove the skips and put the couple of Windows/nat tests in their own file. (TestPortMappedHairpin was the only dual-platform test, it's now got two versions.) Signed-off-by: Rob Murray <[email protected]>
robmry
force-pushed
the
split_bridge_and_nat_tests
branch
from
f333bae to
fca57ac
Compare
| network.CreateNoError(ctx, t, c, serverNetName, network.WithDriver(driverName)) | ||
| network.CreateNoError(ctx, t, c, serverNetName) |
There was a problem hiding this comment.
Not critical, as bridge is the default, but I wonder if we should've kept a network.WithDriver("bridge") here; I see we do for various other tests, and it would keep the test closer to what it was before this PR.
There was a problem hiding this comment.
Yes, 3 of the oldest of 14 test functions in this file explicitly use WithDriver.
Because of the split between Linux/Windows tests this PR introduced - this test doesn't need to create a nat network any more. So, I removed its WithDriver to reduce the clutter and make it more like the other tests. As you say, bridge is the default, so there's no need to specify it.
(Unlike those older tests, the newer ones also omit defaults applied by the test helper functions, like container.WithImage("busybox:latest"), container.WithCmd("top") in container.Run.)
I can put it back if you like? (But I'm not sure it'd be better!) Or, I could put it back, then remove it again in a second commit in this PR?
There was a problem hiding this comment.
It's probably fine to not update it. My thinking was that existing tests use it; it's slightly more explicit what we're expecting to test. Also considering if we would ever make the default configurable, but I guess there will be many tests to update in that case, so not critical.
So yeah, not a blocker.
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
LGTM, but left one minor comment
3 participants
- What I did
Split Linux/bridge and Windows/nat integration tests
Most tests in
integration/networking/bridge_test.goare skipped on Windows - and I want to add a test that uses helpers that aren't implemented on Windows.- How I did it
bridge_test.go->bridge_linux_test.go, remove the skips and put the couple of Windows/nat tests in their own file.TestPortMappedHairpinwas the only dual-platform test, it's now got two versions.- How to verify it
Run the tests.
- Description for the changelog