-
Notifications
You must be signed in to change notification settings - Fork 18.6k
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.
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
integration-cli: Skip tests which require daemon/cli to be on the same host #10889
Conversation
@@ -374,6 +374,8 @@ func TestCpUnprivilegedUser(t *testing.T) { | |||
} | |||
|
|||
func TestCpVolumePath(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like only the bind-mounting parts of this are really "same host assuming", but that the rest of the "testing docker cp
" logic is not only agnostic of that but would be useful client testing too -- do you think there'd be value in splitting this test instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tianon agreed. but the part you're gonna gain by splitting is what other cp tests like TestCpRelativePath, TestCpAbsolutePath, TestCpToDot are already testing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, good point. 👍
Overall not a totally insane list -- I think some of these can be split up a little to improve our Windows test coverage too, but generally on a reasonable track IMO. |
SGTM |
@tianon what's up here. LGTY? |
LGTM (although needs a rebase) |
Some integration-cli tests assume daemon and cli are running on the same machine and therefore they examine side effects of executed docker commands on docker host by reading files or running other sort of commands. In case of windows/darwin CLI tests these provide little or no value and should be OK to skip. List of skipped tests: - `TestContainerNetworkMode` - `TestCpVolumePath` - `TestCreateVolumesCreated` - `TestBuildContextCleanup` - `TestBuildContextCleanupFailedBuild` - `TestLinksEtcHostsContentMatch` - `TestRmContainerWithRemovedVolume` - `TestRunModeIpcHost` - `TestRunModeIpcContainer` - `TestRunModePidHost` - `TestRunNetHost` - `TestRunDeallocatePortOnMissingIptablesRule` - `TestRunPortInUse` - `TestRunPortProxy` - `TestRunMountOrdering` - `TestRunModeHostname` - `TestRunDnsDefaultOptions` - `TestRunDnsOptionsBasedOnHostResolvConf` - `TestRunResolvconfUpdater` - `TestRunVolumesNotRecreatedOnStart` Signed-off-by: Ahmet Alp Balkan <ahmetalpbalkan@gmail.com>
@tianon @jfrazelle @LK4D4 rebased. |
LGTM |
waiting on janky ;) |
…skips integration-cli: Skip tests which require daemon/cli to be on the same host
❤️ |
Some integration-cli tests assume daemon and cli are running on the same host machine and therefore they examine side effects of executed docker commands on docker host by reading files or running other sort of commands.
In case of windows/darwin CLI tests these provide little or no value to CLI testing and should be OK to skip. There's another set of such tests I haven't skipped in this PR since those might be actually fixable with some workarounds but I think the following tests require daemon and cli to be on the same host machine.
List of skipped tests:
TestContainerNetworkMode
TestCpVolumePath
TestCreateVolumesCreated
TestBuildContextCleanup
TestBuildContextCleanupFailedBuild
TestLinksEtcHostsContentMatch
TestRmContainerWithRemovedVolume
TestRunModeIpcHost
TestRunModeIpcContainer
TestRunModePidHost
TestRunNetHost
TestRunDeallocatePortOnMissingIptablesRule
TestRunPortInUse
TestRunPortProxy
TestRunMountOrdering
TestRunModeHostname
TestRunDnsDefaultOptions
TestRunDnsOptionsBasedOnHostResolvConf
TestRunResolvconfUpdater
TestRunReuseBindVolumeThatIsSymlink
🆕TestRunVolumesNotRecreatedOnStart
Signed-off-by: Ahmet Alp Balkan ahmetalpbalkan@gmail.com
cc: @unclejack @tianon @jfrazelle @icecrime @crosbymichael