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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

added entry for running unit tests with junit report #39638

Merged
merged 1 commit into from
Aug 2, 2019
Merged

added entry for running unit tests with junit report #39638

merged 1 commit into from
Aug 2, 2019

Conversation

tiborvass
Copy link
Contributor

Signed-off-by: Andrew Hsu andrewhsu@docker.com
(cherry picked from commit 30d1bf8)
Signed-off-by: Tibor Vass tibor@docker.com

#39637

thaJeztah
thaJeztah reviewed Jul 31, 2019
View reviewed changes
hack/dockerfile/install/gojunitreport.installer Outdated

GOJUNITREPORT_COMMIT=af01ea7f8024089b458d804d5cdf190f962a9a0c

install_gojunitreport() {
Copy link
Member

Choose a reason for hiding this comment

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

Are there advantages to using this instead of gotestsum? docker/cli#1639

Copy link
Member

Choose a reason for hiding this comment

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

i'll have a look. i started with go-junit-report because that's what i've used in the past and was easiest to setup.

Copy link
Member

Choose a reason for hiding this comment

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

i decided to use gotestsum after getting to know it because it's better

@andrewhsu andrewhsu force-pushed the unit-junit branch 2 times, most recently from 849a6f6 to 927ea39 Compare July 31, 2019 01:39
@andrewhsu
Copy link
Member

A successful run with test results: https://ci.docker.com/public/blue/organizations/jenkins/moby/detail/PR-39638/3/tests

@thaJeztah thaJeztah added this to In progress in Improving CI via automation Jul 31, 2019
@andrewhsu andrewhsu force-pushed the unit-junit branch 3 times, most recently from 7e989e3 to 8954289 Compare July 31, 2019 23:34
@thaJeztah
Copy link
Member

ping @vdemeester ptal

@thaJeztah
Copy link
Member

interesting failure on janky

16:40:05 === FAIL: pkg/pubsub TestSendToOneSub (0.00s)
16:40:05     publisher_test.go:17: expected message hi but received hi

@andrewhsu
Copy link
Member

@thaJeztah i needed to introduce a failure to see how junit failure reporting looks: https://ci.docker.com/public/job/moby/job/PR-39638/9/testReport/github/com_docker_docker_pkg_pubsub/TestSendToOneSub/

@andrewhsu
run unit tests and generate junit report
42f0a0d 
Signed-off-by: Andrew Hsu <andrewhsu@docker.com>
@andrewhsu
Copy link
Member

I think this is ready for merge. This PR adds another parallel PR check stage in the Jenkinsfile for just running the unit tests via hack/test/unit on x86_64. The result of the stage will produce 3 artifacts in a unit-bundles.tar.gz file that will be stored with the job run, e.g. https://ci.docker.com/public/blue/organizations/jenkins/moby/detail/PR-39638/10/artifacts

The contents of the bundle are:

  • go-test-report.json - the go test -json output
  • junit-report.xml - the junit format of the test result for consumption by jenkins
  • profile.out - the coverage report of the unit test runs

The Jenkinsfile has been updated in this PR to consume the junit-report.xml for making graphs and tables of the result like so: https://ci.docker.com/public/job/moby/job/PR-39638/10/testReport/github/

The graphs and tables will help developers quickly locate the info of the failing unit tests instead of searching for the error on the verbose console output page of the jenkins job.

To accomplish this, I had to add gotestsum to the Dockerfile image because it can run the unit tests and generate the junit-report.xml.

@tiborvass tiborvass marked this pull request as ready for review August 1, 2019 22:38
@tiborvass tiborvass requested a review from tianon as a code owner August 1, 2019 22:38
@andrewhsu andrewhsu mentioned this pull request Aug 1, 2019
Closed
@tiborvass
Copy link
Contributor Author

gotestsum works great with go test, but will break for gocheck used by integration-cli. We can discuss that separately.

@tiborvass
Copy link
Contributor Author

PTAL @thaJeztah @vdemeester

andrewhsu
andrewhsu approved these changes Aug 1, 2019
View reviewed changes
Copy link
Member

@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 🤳🏼

This was referenced Aug 1, 2019
Open
Closed
Improving CI automation moved this from In progress to Reviewer approved Aug 2, 2019
vdemeester
vdemeester approved these changes Aug 2, 2019
View reviewed changes
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

@zelahi
Copy link
Contributor

zelahi commented Aug 2, 2019

LGTM

@vdemeester vdemeester merged commit 4e83c90 into moby:master Aug 2, 2019
Improving CI automation moved this from Reviewer approved to Done Aug 2, 2019
@andrewhsu andrewhsu mentioned this pull request Aug 6, 2019
Open
8 tasks
This was referenced Aug 19, 2019
Merged
Merged
@vikramhh vikramhh mentioned this pull request Sep 22, 2019
Closed
@thaJeztah thaJeztah mentioned this pull request Oct 7, 2019
Closed
@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

@thaJeztah thaJeztah thaJeztah left review comments

@vdemeester vdemeester vdemeester approved these changes

@andrewhsu andrewhsu andrewhsu approved these changes

@seemethere seemethere seemethere approved these changes

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

Assignees
No one assigned
Labels
area/testing process/cherry-picked status/0-triage
Projects
Improving CI
  
Done
Milestone
20.10.0
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@tiborvass @andrewhsu @thaJeztah @zelahi @vdemeester @seemethere @GordonTheTurtle