Skip to content

hack: have integration-cli use gotestsum codepath#39911

Merged
cpuguy83 merged 2 commits intomoby:masterfrom
tiborvass:gotestsum-integration-cli
Sep 19, 2019
Merged

hack: have integration-cli use gotestsum codepath#39911
cpuguy83 merged 2 commits intomoby:masterfrom
tiborvass:gotestsum-integration-cli

Conversation

@tiborvass
Copy link
Contributor

@tiborvass tiborvass commented Sep 11, 2019 •
edited
Loading

Signed-off-by: Tibor Vass tibor@docker.com

This also removes the master test named Test, replaced by one test per suite. Review last commit for that.

@tiborvass tiborvass requested a review from tianon as a code owner September 11, 2019 22:04
@andrewhsu andrewhsu mentioned this pull request Sep 11, 2019
Open
8 tasks
@thaJeztah
Copy link
Member

thaJeztah commented Sep 11, 2019

Screenshot 2019-09-12 at 01 02 34

It's showing under the amd64 listing (wondering if we should change the others to also be under there)

Screenshot 2019-09-12 at 01 04 25

This is a bit confusing; Test is shown twice, and as a separate test

Screenshot 2019-09-12 at 01 04 52

@tiborvass
Copy link
Contributor Author

tiborvass commented Sep 12, 2019 •
edited
Loading

@thaJeztah it's because the integration-cli is running one half at a time, so two runs total. If I can get rid of test/suite package altogether, this may be fixed by having TestDockerSuite be a regular Go test.

@tiborvass
Copy link
Contributor Author

tiborvass commented Sep 12, 2019

@thaJeztah PTAL

@tiborvass tiborvass force-pushed the gotestsum-integration-cli branch 2 times, most recently from 8a2a607 to f95f468 Compare September 17, 2019 22:03
andrewhsu
andrewhsu approved these changes Sep 17, 2019
View reviewed changes
Copy link
Contributor

@andrewhsu andrewhsu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tiborvass
Copy link
Contributor Author

tiborvass commented Sep 18, 2019

@cpuguy83 @thaJeztah Fixed the issues. Including the test names: there is no more master test named Test. It's in the last commit.

thaJeztah
thaJeztah reviewed Sep 18, 2019
View reviewed changes
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Left some comments, but not a blocker per-se; we can iterate over this and improve.

I also noticed that the integration tests show up as amd64.integration in the top level;

Screenshot 2019-09-18 at 13 29 54

Screenshot 2019-09-18 at 13 30 02

Whereas the integration-cli ones are nested under amd64. We should have a look why, and either group all under amd64, or have both shown as amd64.integration and amd64.integration-cli.

I did a quick comparison between the XML for both; Not sure what's causing the difference in grouping; perhaps because integration has three elements (amd64.integration.<name>)? Or could it be because of the hyphen in integration-cli ?

amd64-integration-cli-junit-report.xml:

<testsuite tests="1219" failures="0" time="4507.577000" name="amd64.integration-cli">
	<testcase classname="amd64.integration-cli" name="TestDockerSuite/DockerSuite/TestAPIClientVersionOldNotSupported" time="0.010000"></testcase>

amd64-integration-build-junit-report.xml

<testsuite tests="32" failures="0" time="169.590000" name="amd64.integration.build">
	<testcase classname="amd64.integration.build" name="TestCgroupNamespacesBuild" time="5.100000"></testcase>

integration-cli/check_test.go Outdated
Copy link
Member

@thaJeztah thaJeztah Sep 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have a tracking issue for this (#16087 / #15992)

Copy link
Contributor Author

@tiborvass tiborvass Sep 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm only refactoring, but sure I don't know what happened there.

Copy link
Member

@thaJeztah thaJeztah Sep 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

neither do I, but perhaps something @ddebroy @vikramhh can look at

Tibor Vass added 2 commits September 18, 2019 18:26
hack: have integration-cli use gotestsum codepath
84928be 
Signed-off-by: Tibor Vass <tibor@docker.com>
integration-cli: move each test suite to its own TestX testing function
f1c1cd4 
Signed-off-by: Tibor Vass <tibor@docker.com>
@tiborvass tiborvass force-pushed the gotestsum-integration-cli branch from f95f468 to f1c1cd4 Compare September 18, 2019 18:27
@tiborvass
Copy link
Contributor Author

tiborvass commented Sep 18, 2019

@thaJeztah rebased and fixed duplicate suite name issue.

@cpuguy83
Copy link
Member

cpuguy83 commented Sep 18, 2019

Oh my

2019-09-18T20:34:13.543Z] WARN [runner/golint] Golint: can't lint 4 files: no file name for file &{Doc:<nil> Package:19518851 Name:quota Decls:[0xc023af1e00 0xc023af1e80 0xc023af1f00 0xc023af1f80 0xc023d78f00 0xc023d79440] Scope:scope 0xc0257cb1e0 {

[2019-09-18T20:34:13.543Z] 	var ErrQuotaNotSupported

[2019-09-18T20:34:13.543Z] 	type errQuotaNotSupported

[2019-09-18T20:34:13.543Z] }

[2019-09-18T20:34:13.543Z]  Imports:[0xc023d78b10] Unresolved:[errdefs nil string] Comments:[0xc01cee2bc0 0xc01cee2d40]} 

[2019-09-18T20:34:45.615Z] fatal error: runtime: out of memory

@andrewhsu
Copy link
Contributor

andrewhsu commented Sep 18, 2019

@cpuguy83 yeah, we're investigating that. i think the node that this PR ran on had a process from an old job that was still using resources.

@tiborvass tiborvass mentioned this pull request Sep 18, 2019
Merged
@tiborvass
Copy link
Contributor Author

tiborvass commented Sep 18, 2019

@cpuguy83 #39957

@tiborvass
Copy link
Contributor Author

tiborvass commented Sep 19, 2019

Failure in windows RS5 is unrelated:

[2019-09-18T23:03:06.632Z]     --- FAIL: TestDockerSuite/TestPsListContainersFilterExited (8.67s)

[2019-09-18T23:03:06.632Z]         docker_cli_ps_test.go:454: assertion failed: expression is false: strings.Contains(out, strings.TrimSpace(firstZero))

@tiborvass
Copy link
Contributor Author

tiborvass commented Sep 19, 2019

@thaJeztah @cpuguy83 PTAL

thaJeztah
thaJeztah approved these changes Sep 19, 2019
View reviewed changes
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

cpuguy83
cpuguy83 approved these changes Sep 19, 2019
View reviewed changes
Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@cpuguy83 cpuguy83 merged commit 0f18e43 into moby:master Sep 19, 2019
@thaJeztah thaJeztah mentioned this pull request Sep 26, 2019
Merged
@thaJeztah thaJeztah added this to the 20.03.0 milestone Apr 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cpuguy83 cpuguy83 cpuguy83 approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

@tianon tianon Awaiting requested review from tianon tianon is a code owner

+1 more reviewer

@andrewhsu andrewhsu andrewhsu approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area/testing process/cherry-picked status/2-code-review

Projects

None yet

Milestone

20.10.0

Development

Successfully merging this pull request may close these issues.

4 participants

@tiborvass @thaJeztah @cpuguy83 @andrewhsu