Merged
Conversation
thaJeztah
added
area/testing
process/cherry-pick
status/2-code-review
and removed
status/0-triage
labels
Member
|
/cc @vdemeester |
thaJeztah
reviewed
Aug 26, 2019
Member
|
On vacation still but saw this come in my inbox, really awesome!
Thanks for doing this!
Will review (if it's still needed) next week.
|
thaJeztah
reviewed
Aug 26, 2019
Member
|
We can now probably also remove the moby/internal/test/daemon/daemon.go Lines 37 to 42 in e554fb2 |
tiborvass
force-pushed
the
bye-bye-gocheck
branch
from
66a65b0 to
c030788
Compare
tiborvass
commented
Aug 26, 2019
hack/make/.integration-test-helpers
Outdated
Contributor
Author
There was a problem hiding this comment.
Please note that i removed the global timeout in favor of per-test timeout (which doesn't work for now).
tiborvass
commented
Aug 26, 2019
Contributor
Author
There was a problem hiding this comment.
I should probably explain why I removed this test in its own commit message: this test used to test an API regression which could possibly be a unit test. However today it will always silently succeed because PublicPort and IP are not even nullable types (not a pointer), and yet checker.NotNil succeeds silently instead of panicking.
Basically what happened is that this test got refactored to start using the API types and API client library instead of custom types and stdlib's http functions.
Contributor
Author
There was a problem hiding this comment.
I added it in the commit message
Member
There was a problem hiding this comment.
See my other comments; looking at this again, I guess these were using checker.NotNil because the value itself was not important (but must be set), so perhaps we can change these to value != "" (e.g.)
thaJeztah
requested changes
Aug 26, 2019
Member
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
made a first pass, and left some comments inline; also wondering if we can change c *testing.T to t *testing.T everywhere (there might be some where t is in use in the test, but we could fix those up)
Member
There was a problem hiding this comment.
Looks like "actual" and "expected" are flipped here
Contributor
Author
There was a problem hiding this comment.
equality is symmetric
Contributor
There was a problem hiding this comment.
equality is symmetric
True, but this func is documented as
func Equal(t TestingT, expected, actual interface{}, msgAndArgs ...interface{}) booland the error message it generates might use the "expected" and "actual" terms, so it makes sense to have them in the correct order.
Member
There was a problem hiding this comment.
the fmt.Sprintf(..) is not needed, assert.Equal() already takes a format-string;
moby/vendor/gotest.tools/assert/assert.go
Line 256 in c147e9e
func TestFoo(t *testing.T) {
assert.Equal(t, "one", "two", "bla %s bla", "bla")
}=== RUN TestFoo
--- FAIL: TestFoo (0.00s)
debug_test.go:48: assertion failed: one (string) != two (string): bla bla bla
FAIL
Process finished with exit code 1
Contributor
Author
There was a problem hiding this comment.
Sure, but my script is no AI yet ;)
Member
There was a problem hiding this comment.
It could probably remove all check.Commentf(...), but we can do in a follow-up
Member
There was a problem hiding this comment.
For a follow-up; wondering if we should just use == for comparison instead of assert.Equal, which would me more consistent with != below (and take any weird comparison bugs in the assertion library)
Member
There was a problem hiding this comment.
I would prefer to use assert.Equal over being consistent, because in the failure case the logging is cleaner. It's just unfortunate that the assert library does not have a generic Not assertion.
Member
There was a problem hiding this comment.
That said, a NotEqual comparison should not be terribly hard, except the comparison implementation details (like output formatting) are not exported.
Member
There was a problem hiding this comment.
Looks like you both moved this one (_unix_test -> _test) and removed the !windows build-tag ?
Contributor
Author
There was a problem hiding this comment.
This is so that DockerExternalVolumeSuite is reachable when GOOS=windows.
Member
There was a problem hiding this comment.
Should we replace this with a !windows buildtag, or a check if daemon != client platform?
Contributor
Author
There was a problem hiding this comment.
I think there is value in seeing all the suites in one place. We could maybe have the SetUp hook skip the test based on runtime.GOOS instead of conditionals in Test()
integration-cli/docker_utils_test.go
Outdated
Member
There was a problem hiding this comment.
assert.NilError(c, err, ...) (we can probably reach if there's others like this)
Contributor
Author
There was a problem hiding this comment.
Sure, just not super important, as long as it works as intended i'm fine. The goal is to remove gocheck while keeping the tests correct.
tiborvass
force-pushed
the
bye-bye-gocheck
branch
2 times, most recently
from
b664aae to
706cd7f
Compare
Contributor
Author
Sure, I just thought it would be more prudent to keep it this way. Also you might be just as surprised as I was when I saw that the stdlib also uses |
Member
Hm.. yes, that's surprising. They seem to not be consistent there (other cases on the same page use |
Member
|
Looks like there's some leftover And in the e2e-run.sh; Line 42 in a9dc697 |
Member
|
found some more |
Contributor
Author
|
@thaJeztah thanks, will do |
tiborvass
force-pushed
the
bye-bye-gocheck
branch
2 times, most recently
from
ded3390 to
faf3b22
Compare
Member
|
oh, whoops; needs a rebase 😓 - should not be too difficult though |
tiborvass
force-pushed
the
bye-bye-gocheck
branch
from
faf3b22 to
97db1cc
Compare
thaJeztah
requested changes
Sep 3, 2019
Member
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
sorry, missed that you rebased; looks like build is failing (left comments inline)
internal/test/suite/suite.go
Outdated
Member
There was a problem hiding this comment.
Build is failing here; https://ci.docker.com/public/job/moby/job/PR-39799/7/execution/node/156/log/
23:20:11 internal/test/suite/suite.go:1::warning: file is not goimported (goimports)
23:20:11 internal/test/suite/suite.go:7:2:warning: "fmt" imported but not used (unconvert)
23:20:11 internal/test/suite/suite.go:12:2:warning: "time" imported but not used (gosimple)
97db1cc to
889b793
Compare
Member
This looks a bit weird; looks like it sees Looks like we may need some throttling on these; Details |
thaJeztah
reviewed
Sep 3, 2019
added 4 commits
September 9, 2019 21:09
- remove -check.* flags - use (per-test) -timeout flag - allow user to override TEST_SKIP_* regardless of TESTFLAGS - remove test-imports validation Signed-off-by: Tibor Vass <[email protected]>
Signed-off-by: Tibor Vass <[email protected]>
Signed-off-by: Tibor Vass <[email protected]>
Signed-off-by: Tibor Vass <[email protected]>
tiborvass
force-pushed
the
bye-bye-gocheck
branch
from
8564b1f to
4424e7d
Compare
Member
|
`testing.TB`
…On Mon, Sep 9, 2019 at 2:29 PM Tibor Vass ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In integration-cli/cli/cli.go
<#39799 (comment)>:
> @@ -30,6 +30,10 @@ type testingT interface {
Fatalf(string, ...interface{})
}
+type TestingT interface {
Yes because it is used in definition of dockerCmd which is used in both
benchmarks and tests. If I replace it with *testing.T I have this error:
integration-cli/docker_cli_logs_bench_test.go:11:21: cannot use c (type *testing.B) as type *testing.T in argument to dockerCmd
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#39799?email_source=notifications&email_token=AAGDCZXUAKDWYK7Z5TNZLX3QI252PA5CNFSM4IPUIUS2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCEEOHUY#discussion_r322463343>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAGDCZQVMYLC4LW2UIGFIN3QI252PANCNFSM4IPUIUSQ>
.
--
- Brian Goff
|
added 2 commits
September 9, 2019 21:47
Signed-off-by: Tibor Vass <[email protected]>
Signed-off-by: Tibor Vass <[email protected]>
tiborvass
force-pushed
the
bye-bye-gocheck
branch
from
a2c58aa to
8e6adce
Compare
cpuguy83
reviewed
Sep 9, 2019
added 2 commits
September 10, 2019 00:25
Signed-off-by: Tibor Vass <[email protected]>
Signed-off-by: Tibor Vass <[email protected]>
tiborvass
force-pushed
the
bye-bye-gocheck
branch
from
8e6adce to
231ed42
Compare
Contributor
Author
|
@cpuguy83 it's green! |
thaJeztah
approved these changes
Sep 10, 2019
Member
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
LGTM
left some thoughts about some asserts/tests we removed, but for a follow-up
| c.Assert(nr.EnableIPv6, checker.Equals, false) | ||
| c.Assert(nr.IPAM.Driver, checker.Equals, "default") | ||
| c.Assert(len(nr.IPAM.Config), checker.Equals, 1) | ||
| c.Assert(nr.IPAM.Config[0].Subnet, checker.NotNil) |
Member
There was a problem hiding this comment.
Perhaps we should create a tracking issue; I can imagine this was meant to check for a non-empty value, so could be changed to, e.g. nr.IPAM.Config[0].Subnet != ""
| c.Assert(nr.IPAM.Driver, checker.Equals, "default") | ||
| c.Assert(len(nr.IPAM.Config), checker.Equals, 1) | ||
| c.Assert(nr.IPAM.Config[0].Subnet, checker.NotNil) | ||
| c.Assert(nr.IPAM.Config[0].Gateway, checker.NotNil) |
Member
There was a problem hiding this comment.
See my other comments; looking at this again, I guess these were using checker.NotNil because the value itself was not important (but must be set), so perhaps we can change these to value != "" (e.g.)
Member
|
Only failure is a flaky test on Windows RS1; |
Member
|
🎉 |
This was referenced Sep 23, 2019
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This will get rid of the ancient go-check framework and replaces it with gotest.tools because:
I know there are many commits but I tried to make each commit easily reviewable on purpose, some of them are automated.
The commits prefixed with
rm-gocheck:have been made by a script that's in the commit message.The main structure of this PR is as follows:
TODO:
TESTFLAGS='-timeout 5s -test.run /DockerDaemonSuite/TestDaemonKillLiveRestoreWithPlugins)cc @thaJeztah @andrewhsu @cpuguy83