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.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

integration-cli: add const to skip daemon-requiring cli tests #10785

Merged
merged 1 commit into from Feb 18, 2015
Merged

integration-cli: add const to skip daemon-requiring cli tests #10785

merged 1 commit into from Feb 18, 2015

Conversation

ahmetb
Copy link
Contributor

@ahmetb ahmetb commented Feb 13, 2015

If DOCKER_CLIENTONLY is set for test-integration-cli, we don't set the daemon build tag. const isRemoteDaemon will help us skip tests which require daemon and cli to be running on the same machine without moving them to a separate file and accidentally lose track of them.

As a start I skipped some TestLinks* and TestNetworkNat tests. These are the ones verifying the functionality of the cli commands by examining its daemon-side side effects, and thus assume daemon is on the same machine with cli.

Doing such will disable them from being executed for windows/darwin CLIs but it will still run as a part of Linux tests. When Windows daemon starts to light up and can start to compile without DOCKER_CLIENTONLY, these tests will need to be fixed to verify things "the windows way".

I will be sending more test skips in this integration-cli suite as I go, this is just a beginning. 馃憤

Signed-off-by: Ahmet Alp Balkan ahmetb@microsoft.com
Label: #windows
Cc: @tianon @unclejack @jfrazelle @jhowardmsft @icecrime @swernli

@tianon
Copy link
Member

@tianon tianon commented Feb 13, 2015

+1 to this approach -- makes it nice and obvious when and why tests are being excluded

@icecrime
Copy link
Contributor

@icecrime icecrime commented Feb 14, 2015

Smart! 馃憤

One nitpick: we could use a func requiresLocalDaemon(t *testing.T) which skips with the appropriate message when necessary to avoid duplication.

@ahmetb
Copy link
Contributor Author

@ahmetb ahmetb commented Feb 14, 2015

@icecrime that'd be smart. I can change that. however we might want to provide custom skip messages explaning why we're skipping, so it probably should look like func requiresLocalDaemon(t *testing.T, reason string).

Skip reason is not always 鈥 Test assumes docker host runs on the same machine as cli, skip this test鈥 as I sent in this PR, there'll be more skips due to different reasons.

@tiborvass
Copy link
Collaborator

@tiborvass tiborvass commented Feb 14, 2015

@ahmetalpbalkan what would be another reason?

Either way, once we use test suites, we could consolidate some of these skips in the pre-hook.

@ahmetb
Copy link
Contributor Author

@ahmetb ahmetb commented Feb 14, 2015

@tiborvass some tests require bash/cat/tee etc and currently all run on msys bash for windows CI. We may think of running some with cmd.exe /c "docker.exe [...]".

In that case ineligible ones should be skipped. But immediate priority is skipping those tests assume there's a local daemon on the machine.

@icecrime
Copy link
Contributor

@icecrime icecrime commented Feb 14, 2015

I agree we'll need multiple "skipping reasons", but maybe we should make them const to avoid repeating those strings throughout the tests? Here's a suggestion (maybe overkill, I don't know it's morning):

type TestRequirement int

const (
    SameHostDaemon TestRequirement = iota
)

var testRequirements = map[TestRequirement]struct {
    Condition bool
    Message   string
}{
    SameHostDaemon: {isRemoteDaemon, "Test requires docker daemon runs on the same machine as cli"},
}

func testRequires(t *testing.T, requirement TestRequirement) {
    if v, ok := testRequirements[requirement]; !ok {
        t.Fatalf("Invalid test requirement %q", requirement)
    } else if !v.Condition {
        t.Skip(v.Message)
    }
}

func TestSomething(t *testing.T) {
    testRequires(t, SameHostDaemon)
}

Test file here: https://play.golang.org/p/Lp7kOwwCaJ

@tiborvass
Copy link
Collaborator

@tiborvass tiborvass commented Feb 14, 2015

Why not, we could use @icecrime's suggestion.

If DOCKER_CLIENTONLY is set for test-integration-cli, we don't set
the 'daemon' build tag. 'isRemoteDaemon' will help us skip such
tests without a need to move them to a separate file and accidentally
lose track of them.

Added `testRequires` function to skip tests based on predefined
conditions evaluated in runtime. This way we can easily extend test
requirements like:

    testRequires(t, Networking, SameHostDaemon, Linux)

Signed-off-by: Ahmet Alp Balkan <ahmetb@microsoft.com>
@ahmetb
Copy link
Contributor Author

@ahmetb ahmetb commented Feb 17, 2015

@icecrime nice idea. I polished this a bit and came up something a bit extensible and type-safe:

type TestCondition func() bool

type TestRequirement struct {
    Condition   TestCondition
    SkipMessage string
}

// List test requirements
var (
    SameHostDaemon = TestRequirement{
        func() bool { return isLocalDaemon },
        "Test requires docker daemon to runs on the same machine as CLI",
    }
)

// testRequires checks if the environment satisfies the requirements
// for the test to run or skips the tests.
func testRequires(t *testing.T, requirements ...TestRequirement) {
    for _, r := range requirements {
        if !r.Condition() {
            t.Skip(r.SkipMessage)
        }
    }
}

// in tests, we can easily say:
testRequires(t, SameHostDaemon, InContainer, Network)

Please comment on design. @tianon @jfrazelle

@jessfraz
Copy link
Contributor

@jessfraz jessfraz commented Feb 18, 2015

Oooo I like LGTM

@icecrime
Copy link
Contributor

@icecrime icecrime commented Feb 18, 2015

So clean, love it 馃憤 LGTM

@tianon
Copy link
Member

@tianon tianon commented Feb 18, 2015

Interesting approach!

LGTM!

@icecrime
Copy link
Contributor

@icecrime icecrime commented Feb 18, 2015

Skipping test review for tests-only PR.

icecrime pushed a commit that referenced this issue Feb 18, 2015
integration-cli: add const to skip daemon-requiring cli tests
@icecrime icecrime merged commit 306bb28 into moby:master Feb 18, 2015
1 check passed
@ahmetb ahmetb deleted the win-cli/TestLinks-skip branch Feb 18, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants