Use forked gocheck #21017

cpuguy83 · 2016-03-08T03:59:21Z

added support for per-check timeouts (upstream is non-responsive)
set 5m per-check timeout

The original PR I made to go-check had a bad rebase (and difficult to recover from), so opened a new on here: go-check/check#80

Another PR for consideration is go-check/check#70 which adds parallel tests -- this is not included here as I have not merged it into my fork's master branch in consideration of the per-check timeout being a bit more pertinent to Docker's test suite right now.

The last merge to go-check/check was 3 months ago, and that merge also broke the test suite :(

cpuguy83 · 2016-03-08T04:01:19Z

ping @tiborvass @icecrime @tianon

jessfraz · 2016-03-08T16:09:59Z

ah saddness that upstream :(

thaJeztah · 2016-03-08T16:37:25Z

I see there's also an open PR from @brahmaroutu go-check/check#47 should we include that in our fork?

tianon · 2016-03-08T16:48:05Z

Oh Go ecosystem 😢

For posterity's sake, can we enumerate some of the issues we've seen with upstream here? (perhaps links to PRs that have gone stale?) That'll be helpful to point anyone asking why we use a fork (or who traces it back to a commit and thus PR), and could also be an OK way to check whether we could consider switching back sometime later.

cpuguy83 · 2016-03-08T16:51:35Z

@tianon Yes, can definitely do that. Unfortunately I'll have to re-create the branch since I managed to push -f a change on it that really should have gone in another branch ( 😢 ). Luckily the changes required for this are rather small other than adding the tests.

thaJeztah · 2016-03-08T17:00:43Z

Related PR upstream: go-check/check#70 (and the one I mentioned above possibly)

tianon · 2016-03-08T17:10:07Z

@cpuguy83 oh, I just meant here -- it's pretty easy IMO to trace a commit back to a PR (by going to https://github.com/.../commit/... and clicking the #xxx link), so @thaJeztah's comment is good enough for me 👍

cpuguy83 · 2016-03-08T17:20:56Z

Updated the PR to list the PRs.

tianon · 2016-03-08T17:53:27Z

LGTM 😞

icecrime · 2016-03-08T23:43:13Z

Not sure what's wrong here:

15:27:32 can't load package: package github.com/docker/docker: no buildable Go source files in /go/src/github.com/docker/docker

thaJeztah · 2016-03-10T11:42:24Z

ping @cpuguy83 any idea what those failures are? TL;DR got a lot of WindowsTP4 builds timing out, so wondering if we should up the timeout for those, or if we can get this PR merged soon

cpuguy83 · 2016-03-10T11:52:48Z

I have literally no idea.
Note that experimental also failed just like janky, even though it is marked as successful.

thaJeztah · 2016-03-10T11:57:36Z

ping @mlaventure perhaps we should up the timeout for WindowsTP4 while we figure out what's wrong here?

mlaventure · 2016-03-10T16:18:51Z

This one is not a timeout issue.

It looks like the copy of the sources into the container didn't happen for some reason.

But I can see it happening in the log:

04:26:34 Step 50 : COPY . /go/src/github.com/docker/docker
04:26:38  ---> ba1e4201c70f

Edit: adding missing not

cpuguy83 · 2016-03-10T16:20:32Z

Yeah, it's super strange and it's only happening on this PR.
Tried rebuilding multiple times... rebasing and pushing, etc.

cpuguy83 · 2016-03-10T16:21:43Z

And win2lin and userns run successfully.

cpuguy83 · 2016-03-10T19:02:02Z

Seems I found the issue. Had to move the check.timeout flag from make.sh to make/integration.
Tests seem to be making it past where they did before.

cpuguy83 · 2016-03-10T20:33:57Z

All 💚

calavera · 2016-03-11T00:36:07Z

LGTM

cyphar · 2016-03-11T15:11:48Z

If this works well, I think we should ditch using the suite timeout at all (it doesn't make sense, because we're always adding new tests, it's more important that if a test stalls we fail it after a timeout -- it doesn't matter how long the whole suite takes as long as it doesn't freeze).

cpuguy83 · 2016-03-11T15:14:56Z

@cyphar Really I believe the -timeout option would be per-test, but since we're hijacking a single testing.T in the integration tests it ends up being for the whole suite.

Instead of removing it, we can still set the timeout option so it also applies to unit tests and such.

tiborvass · 2016-03-11T15:39:10Z

vendor/src/github.com/go-check/check/README.md

-Import it with:
+The intention of this fork is not to change any of the original behavior, but add
+some specific behaviors needed for some of my projects already using this test suite.
+For documentation on the main behavior of go-check see the afformentioned repo.


aforementioned :)

estesp · 2016-03-11T16:58:16Z

Makefile

@@ -45,7 +45,7 @@ DOCKER_ENVS := \
 	-e DOCKER_USERLANDPROXY \
 	-e TESTDIRS \
 	-e TESTFLAGS \
-	-e TIMEOUT
+	-e TIMEOUT \


not going to harm anything at the moment, but this line continuation isn't needed after removing the new variable

Signed-off-by: Brian Goff <[email protected]>

cpuguy83 · 2016-03-11T21:49:58Z

all 💚 again
In order to remove the somewhat duplicated CHECKTIMEOUT (and use TIMEOUT instead), we had to set a super long -timeout for the integration tests.
This is because there is no way to disable go's built-in test timeout, which defaults to calling SIGQUIT after 10mins (when -timeout is not set)

cpuguy83 · 2016-03-11T21:51:15Z

hack/make.sh

@@ -250,6 +248,7 @@ test_env() {
 		DOCKER_REMAP_ROOT="$DOCKER_REMAP_ROOT" \
 		DOCKER_REMOTE_DAEMON="$DOCKER_REMOTE_DAEMON" \
 		GOPATH="$GOPATH" \
+		GOTRACEBACK=all \


This was added so that the panics that go-check produces give a full goroutine dump, otherwise the trace is useless since it's just a trace of the main go-check code, not the timed-out test code.

tianon · 2016-03-11T21:58:10Z

LGTM

tiborvass · 2016-03-11T23:02:28Z

LGTM

Use forked gocheck

thaJeztah · 2016-03-12T10:17:28Z

🎉 thanks @cpuguy83

GordonTheTurtle added the status/0-triage label Mar 8, 2016

cpuguy83 added status/1-design-review and removed status/0-triage labels Mar 8, 2016

icecrime added the status/failing-ci Indicates that the PR in its current state fails the test suite label Mar 8, 2016

cpuguy83 force-pushed the use_forked_gocheck branch from aec0549 to 8b5ffc0 Compare March 10, 2016 03:30

thaJeztah mentioned this pull request Mar 10, 2016

Fix ShouldRestart for on-failure handle #20853

Merged

cpuguy83 force-pushed the use_forked_gocheck branch from 8b5ffc0 to c012200 Compare March 10, 2016 16:23

cpuguy83 removed the status/failing-ci Indicates that the PR in its current state fails the test suite label Mar 10, 2016

calavera added status/2-code-review and removed status/1-design-review labels Mar 11, 2016

tiborvass reviewed Mar 11, 2016
View reviewed changes

cpuguy83 force-pushed the use_forked_gocheck branch 2 times, most recently from 15e9cea to b976da5 Compare March 11, 2016 16:48

estesp reviewed Mar 11, 2016
View reviewed changes

cpuguy83 force-pushed the use_forked_gocheck branch 3 times, most recently from 506703c to d8c5889 Compare March 11, 2016 19:07

cpuguy83 added 2 commits March 11, 2016 14:38

Use forked gocheck and vendor

e6e0837

Signed-off-by: Brian Goff <[email protected]>

use per-check timeouts

11d3f70

Signed-off-by: Brian Goff <[email protected]>

cpuguy83 force-pushed the use_forked_gocheck branch from d8c5889 to 11d3f70 Compare March 11, 2016 19:39

cpuguy83 reviewed Mar 11, 2016
View reviewed changes

tiborvass added status/4-merge and removed status/2-code-review labels Mar 11, 2016

tiborvass added a commit that referenced this pull request Mar 12, 2016

Merge pull request #21017 from cpuguy83/use_forked_gocheck

c73f2c9

Use forked gocheck

tiborvass merged commit c73f2c9 into moby:master Mar 12, 2016

cpuguy83 deleted the use_forked_gocheck branch March 12, 2016 01:58

This was referenced Mar 12, 2016

Adds support for parallel test runs. go-check/check#70

Closed

stacktrace loops infinitely on GCCGO, this fixes the issue go-check/check#47

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use forked gocheck #21017

Use forked gocheck #21017

cpuguy83 commented Mar 8, 2016

cpuguy83 commented Mar 8, 2016

jessfraz commented Mar 8, 2016

thaJeztah commented Mar 8, 2016

tianon commented Mar 8, 2016

cpuguy83 commented Mar 8, 2016

thaJeztah commented Mar 8, 2016

tianon commented Mar 8, 2016

cpuguy83 commented Mar 8, 2016

tianon commented Mar 8, 2016

icecrime commented Mar 8, 2016

thaJeztah commented Mar 10, 2016

cpuguy83 commented Mar 10, 2016

thaJeztah commented Mar 10, 2016

mlaventure commented Mar 10, 2016

cpuguy83 commented Mar 10, 2016

cpuguy83 commented Mar 10, 2016

cpuguy83 commented Mar 10, 2016

cpuguy83 commented Mar 10, 2016

calavera commented Mar 11, 2016

cyphar commented Mar 11, 2016

cpuguy83 commented Mar 11, 2016

tiborvass Mar 11, 2016

estesp Mar 11, 2016

cpuguy83 commented Mar 11, 2016

cpuguy83 Mar 11, 2016

tianon commented Mar 11, 2016

tiborvass commented Mar 11, 2016

thaJeztah commented Mar 12, 2016

Use forked gocheck #21017

Use forked gocheck #21017

Conversation

cpuguy83 commented Mar 8, 2016

cpuguy83 commented Mar 8, 2016

jessfraz commented Mar 8, 2016

thaJeztah commented Mar 8, 2016

tianon commented Mar 8, 2016

cpuguy83 commented Mar 8, 2016

thaJeztah commented Mar 8, 2016

tianon commented Mar 8, 2016

cpuguy83 commented Mar 8, 2016

tianon commented Mar 8, 2016

icecrime commented Mar 8, 2016

thaJeztah commented Mar 10, 2016

cpuguy83 commented Mar 10, 2016

thaJeztah commented Mar 10, 2016

mlaventure commented Mar 10, 2016

cpuguy83 commented Mar 10, 2016

cpuguy83 commented Mar 10, 2016

cpuguy83 commented Mar 10, 2016

cpuguy83 commented Mar 10, 2016

calavera commented Mar 11, 2016

cyphar commented Mar 11, 2016

cpuguy83 commented Mar 11, 2016

tiborvass Mar 11, 2016

Choose a reason for hiding this comment

estesp Mar 11, 2016

Choose a reason for hiding this comment

cpuguy83 commented Mar 11, 2016

cpuguy83 Mar 11, 2016

Choose a reason for hiding this comment

tianon commented Mar 11, 2016

tiborvass commented Mar 11, 2016

thaJeztah commented Mar 12, 2016