use of checkers on `integration-cli/docker_cli_links_test.go` and `integration-cli/docker_cli_links_unix_test.go` #16854

WeiZhang555 · 2015-10-08T15:59:55Z

Use c.Assert instead of condition judgement

Signed-off-by: Zhang Wei zhangwei555@huawei.com

vdemeester · 2015-10-08T16:40:39Z

integration-cli/docker_cli_links_test.go

-	} else if exitCode != 1 {
-		c.Fatalf("run ping failed with errors: %v", err)
-	}
+	c.Assert(exitCode == 0, check.Equals, false, check.Commentf("run ping did not fail"))


You can replace that with :

c.Assert(exitCode, check.Equals, 1, check.Commentf("run ping failed with the following error : %v", err))

MHBauer · 2015-10-08T18:04:44Z

integration-cli/docker_cli_links_test.go

-		c.Fatalf("error output expected 'Could not get container', but got %q instead; err: %v", out, err)
-	}
-
+	c.Assert(err, check.NotNil, check.Commentf("an invalid container target should produce an error"))


this can be just a regular comment
// an invalid container target should produce an error
on the same line or the previous line, and it will be printed out.

WeiZhang555 · 2015-10-09T04:07:39Z

Thank you for your suggestion :) @MHBauer @vdemeester
Refreshed and updated integration file integration-cli/docker_cli_links_unix_test.go too

vdemeester · 2015-10-09T06:53:13Z

integration-cli/docker_cli_links_test.go

@@ -2,8 +2,8 @@ package main

 import (
 	"fmt"
+	"github.com/docker/docker/pkg/integration/checker"


Could you reorganize the imports ? like

import ( "fmt" "regexp" "strings" "github.com/docker/docker/pkg/integration/checker" "github.com/go-check/check" )

WeiZhang555 · 2015-10-09T14:26:22Z

updated :)
@vdemeester

vdemeester · 2015-10-09T14:38:29Z

@WeiZhang555 Great ! Small nit, could you use checker (like checker.Equals, etc…) instead of checker on every checker so it's more coherent ? 😝.

Other than that, looks good 😉.

WeiZhang555 · 2015-10-09T15:55:38Z

@vdemeester
Updated. But check.Commentf has no replacement like checker.Commentf, maybe we should add one more checker.Commentf in pkg/integration/checker/checker.go, WDYT?

vdemeester · 2015-10-09T16:02:39Z

@WeiZhang555 hum, no, just use checker on the "checker" argument (the 2nd) 😝. For now at least 😅.

vdemeester · 2015-10-09T21:47:51Z

@WeiZhang555 seems like docker_cli_links_unix_test.go misses an import of checker (github.com/docker/docker/pkg/integration/checker) 😅

WeiZhang555 · 2015-10-10T01:24:50Z

OK, updated, thank you! @vdemeester

vdemeester · 2015-10-10T11:33:16Z

LGTM 🐱

cpuguy83 · 2015-10-10T12:07:15Z

integration-cli/docker_cli_links_test.go

-	}
-
+	// an invalid container target should produce an error
+	c.Assert(err, checker.NotNil)


Please include out in check.Commentf here, so that if a failure occurs we can see what happened, otherwise we'd generally just get exit status 1 or something from the err.

WeiZhang555 · 2015-10-10T13:53:23Z

@cpuguy83 updated, thank you!

abronan · 2015-10-10T22:47:37Z

@WeiZhang555 needs a rebase, sorry 😞

WeiZhang555 · 2015-10-11T03:10:02Z

@abronan rebased, thank you! :)

vdemeester · 2015-10-11T08:20:23Z

integration-cli/docker_cli_links_test.go

-
+	c.Assert(err, checker.IsNil, check.Commentf("contentTwo: %s", string(contentTwo)))
+	// Host is not present in updated hosts file
+	c.Assert(string(contentTwo), checker.Contains, "onetwo")


Could you move the comment in the end of the line or in a check.Commentf(…) so we don't loose the "context" when this assertion fails ?

// Host is not present in updated hosts file c.Assert(string(contentTwo), checker.Contains, "onetwo")

This won't lose the "context", actually "Host is not present in updated hosts file" will also be printed out when the assertion fails since c.Assert can handle annotation above it as my little test shows.

So that's why I put the context above the c.Assert statement instead of putting it in check.Commentf or in the end of the line, that won't make sentence too long to read.

Will this be acceptable? 😄

ping @vdemeester

Yep yep, I forgot that it handled the comment above too 😅. My bad 😉 👍
The only thing I'm not sure, if when there is a comment above and a check.Commentf 😝.

Yeah, comment above and check.Commentf can coexists, for example:
code:

// err shouldn't be nil // second line // third line c.Assert(err, check.NotNil, check.Commentf("This is comment"))

output:

---------------------------------------------------------------------- FAIL: docker_api_test.go:20: DockerSuite.TestApiGetEnabledCors docker_api_test.go:25: // err shouldn't be nil // second line // third line c.Assert(err, check.NotNil, check.Commentf("This is comment")) ... value = nil ... This is comment ----------------------------------------------------------------------

I quite like this format, what do you think? 😄

Yay, fine by me then 😉. I actually never tried it out 😅, always used check.Commentf 😝.

WeiZhang555 · 2015-10-15T03:58:20Z

Any progress ?

vdemeester · 2015-10-17T12:02:40Z

Still LGTM for me
ping @cpuguy83 @calavera @abronan

cpuguy83 · 2015-10-17T12:06:59Z

integration-cli/docker_cli_links_test.go

-		c.Errorf("/etc/hosts should be a regular file")
-	}
+	// /etc/hosts should be a regular file
+	c.Assert(strings.HasPrefix(out, "-"), checker.Equals, true)


check.Matches instead?
c.Assert(out, check.Matches, '^-')

Note I have not tested.
If Matches doesn't work, then need to make sure to put out in a Commentf.

It seems that ^- doesn't work, but ^-.+ works fine.

WeiZhang555 · 2015-10-17T14:50:47Z

@cpuguy83 updated per your suggestion, please help review, thank you! :)

Part of moby#16756 Use c.Assert instead of condition judgement. Signed-off-by: Zhang Wei <zhangwei555@huawei.com>

cpuguy83 · 2015-10-19T16:10:25Z

LGTM

use of checkers on `integration-cli/docker_cli_links_test.go` and `integration-cli/docker_cli_links_unix_test.go`

GordonTheTurtle added the status/0-triage label Oct 8, 2015

vdemeester reviewed Oct 8, 2015
View reviewed changes

vdemeester added status/2-code-review and removed status/0-triage labels Oct 8, 2015

MHBauer reviewed Oct 8, 2015
View reviewed changes

HuKeping mentioned this pull request Oct 9, 2015

Proposal: use of checkers on Integration test #16756

Closed

82 tasks

WeiZhang555 force-pushed the inte-test branch 2 times, most recently from 6f3ea86 to a9deed5 Compare October 9, 2015 04:05

vdemeester reviewed Oct 9, 2015
View reviewed changes

WeiZhang555 force-pushed the inte-test branch from a9deed5 to 7f56a42 Compare October 9, 2015 14:24

WeiZhang555 force-pushed the inte-test branch from 7f56a42 to b26f324 Compare October 9, 2015 15:53

WeiZhang555 closed this Oct 9, 2015

WeiZhang555 reopened this Oct 9, 2015

WeiZhang555 force-pushed the inte-test branch from b26f324 to e098386 Compare October 9, 2015 16:02

WeiZhang555 force-pushed the inte-test branch from e098386 to 5a29d54 Compare October 10, 2015 01:23

cpuguy83 reviewed Oct 10, 2015
View reviewed changes

WeiZhang555 force-pushed the inte-test branch from 5a29d54 to 4947272 Compare October 10, 2015 13:51

WeiZhang555 force-pushed the inte-test branch from 4947272 to 593dc3a Compare October 11, 2015 03:09

vdemeester reviewed Oct 11, 2015
View reviewed changes

This was referenced Oct 11, 2015

Update integration test integration-cli/docker_cli_rmi_test.go with Assert #16901

Merged

use of checkers on integration-cli/docker_cli_start_test.go #16917

Merged

WeiZhang555 changed the title ~~use of checkers on Integration test~~ use of checkers on integration-cli/docker_cli_links_test.go and integration-cli/docker_cli_links_unix_test.go Oct 12, 2015

cpuguy83 reviewed Oct 17, 2015
View reviewed changes

WeiZhang555 force-pushed the inte-test branch from 593dc3a to bd4ed50 Compare October 17, 2015 14:48

use of checkers on Integration test

97b9223

Part of moby#16756 Use c.Assert instead of condition judgement. Signed-off-by: Zhang Wei <zhangwei555@huawei.com>

WeiZhang555 force-pushed the inte-test branch from bd4ed50 to 97b9223 Compare October 18, 2015 01:01

cpuguy83 added a commit that referenced this pull request Oct 19, 2015

Merge pull request #16854 from WeiZhang555/inte-test

df15523

use of checkers on `integration-cli/docker_cli_links_test.go` and `integration-cli/docker_cli_links_unix_test.go`

cpuguy83 merged commit df15523 into moby:master Oct 19, 2015

WeiZhang555 deleted the inte-test branch October 20, 2015 01:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use of checkers on `integration-cli/docker_cli_links_test.go` and `integration-cli/docker_cli_links_unix_test.go` #16854

use of checkers on `integration-cli/docker_cli_links_test.go` and `integration-cli/docker_cli_links_unix_test.go` #16854

WeiZhang555 commented Oct 8, 2015

vdemeester Oct 8, 2015

MHBauer Oct 8, 2015

WeiZhang555 commented Oct 9, 2015

vdemeester Oct 9, 2015

WeiZhang555 commented Oct 9, 2015

vdemeester commented Oct 9, 2015

WeiZhang555 commented Oct 9, 2015

vdemeester commented Oct 9, 2015

vdemeester commented Oct 9, 2015

WeiZhang555 commented Oct 10, 2015

vdemeester commented Oct 10, 2015

cpuguy83 Oct 10, 2015

WeiZhang555 commented Oct 10, 2015

abronan commented Oct 10, 2015

WeiZhang555 commented Oct 11, 2015

vdemeester Oct 11, 2015

WeiZhang555 Oct 11, 2015

vdemeester Oct 11, 2015

WeiZhang555 Oct 11, 2015

vdemeester Oct 11, 2015

WeiZhang555 Oct 11, 2015

WeiZhang555 commented Oct 15, 2015

vdemeester commented Oct 17, 2015

cpuguy83 Oct 17, 2015

WeiZhang555 Oct 17, 2015

WeiZhang555 commented Oct 17, 2015

cpuguy83 commented Oct 19, 2015

use of checkers on integration-cli/docker_cli_links_test.go and integration-cli/docker_cli_links_unix_test.go #16854

use of checkers on integration-cli/docker_cli_links_test.go and integration-cli/docker_cli_links_unix_test.go #16854

Conversation

WeiZhang555 commented Oct 8, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WeiZhang555 commented Oct 9, 2015

Choose a reason for hiding this comment

WeiZhang555 commented Oct 9, 2015

vdemeester commented Oct 9, 2015

WeiZhang555 commented Oct 9, 2015

vdemeester commented Oct 9, 2015

vdemeester commented Oct 9, 2015

WeiZhang555 commented Oct 10, 2015

vdemeester commented Oct 10, 2015

Choose a reason for hiding this comment

WeiZhang555 commented Oct 10, 2015

abronan commented Oct 10, 2015

WeiZhang555 commented Oct 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WeiZhang555 commented Oct 15, 2015

vdemeester commented Oct 17, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WeiZhang555 commented Oct 17, 2015

cpuguy83 commented Oct 19, 2015

use of checkers on `integration-cli/docker_cli_links_test.go` and `integration-cli/docker_cli_links_unix_test.go` #16854

use of checkers on `integration-cli/docker_cli_links_test.go` and `integration-cli/docker_cli_links_unix_test.go` #16854