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.

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

sharness: Generate JUnit test reports #4530

Merged
merged 10 commits into from
Apr 20, 2018
Merged

Conversation

magik6k
Copy link
Member

@magik6k magik6k commented Dec 28, 2017

This makes sharness generate a JUnit XML report that can be used by Jenkins to view test results in a bit nicer form than a huge txt file (removing the fun of wasting few minutes to find the failure each time one happens). The report is saved as test/sharness/test-results/sharness.xml.

Example report: https://ipfs.io/ipfs/QmZVCjX2L9DrbqYaVeVzTXZZUbyQvTM3mTUmzeMCoN8Pqc/sharness.xml - HTML from junit2html - Jenkins should show failures at the top (like here)

Somewhat depends on ipfs-inactive/jenkins#85

@magik6k magik6k added the kind/test Testing work label Dec 28, 2017
@ghost ghost assigned magik6k Dec 28, 2017
@ghost ghost added the status/in-progress In progress label Dec 28, 2017
@magik6k magik6k added the status/blocked Unable to be worked further until needs are met label Dec 29, 2017
@whyrusleeping
Copy link
Member

(removing the fun of wasting few minutes to find the failure each time one happens)

Aww, but that was my favorite part

@whyrusleeping
Copy link
Member

@magik6k @victorbjelkholm whats the blocker here?

@magik6k
Copy link
Member Author

magik6k commented Jan 4, 2018

I'd like to have test result collection enabled on jenkins before this is merged to make sure it works well with our setup (though I have checked it with local jenkins instance and it did work).

That's done in ipfs-inactive/jenkins#85, though @victorbjelkholm suggests to defer/integrate those changes to #4430 / ipfs-inactive/jenkins#81 (Which will probably take more time, but it needs to be done either way)

TL;DR stuff in this PR Works For Me(tm), I'd like to see it not breaking production Jenkins before it's merged

@magik6k magik6k requested a review from Kubuxu as a code owner March 12, 2018 20:26
@magik6k magik6k removed the status/blocked Unable to be worked further until needs are met label Mar 13, 2018
@magik6k
Copy link
Member Author

magik6k commented Mar 13, 2018

tbh I'd love to have this enabled already, even with the legacy job as it doesn't appear the new one is getting enabled anytime soon, and looking through failed sharness output is getting annoying.

I rebased this, shouldn't break anything, some review would be welcome.

@magik6k magik6k added the need/review Needs a review label Mar 14, 2018
.PHONY: $(T_$(d))

$(d)/aggregate: $(T_$(d))
@echo "*** $@ ***"
@(cd $(@D) && ./lib/test-aggregate-results.sh)
@(cd $(@D) && ./lib/gen-junit-report.sh)
Copy link
Member

Choose a reason for hiding this comment

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

I would make this separate target that corresponds to created file.

.PHONY: $(T_$(d))

$(d)/aggregate: $(T_$(d))
@echo "*** $@ ***"
@(cd $(@D) && ./lib/test-aggregate-results.sh)
.PHONY: $(d)/aggregate

$(d)/junit_xml: $(T_$(d))
Copy link
Member

Choose a reason for hiding this comment

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

I meant, name it test-results/sharness.xml as it produces this file.

@@ -52,7 +61,7 @@ $(d)/deps: $(SHARNESS_$(d)) $$(DEPS_$(d)) # use second expansion so coverage can
test_sharness_deps: $(d)/deps
.PHONY: test_sharness_deps

test_sharness_short: $(d)/aggregate
test_sharness_short: $(d)/aggregate $(d)/junit_xml
Copy link
Member

Choose a reason for hiding this comment

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

Most users won't need the JUnit xmls so there is no reason to create them for them.

test $total_prereq = $ok_prereq
}

+junit_testcase() {
Copy link
Member

Choose a reason for hiding this comment

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

Can we have it noop when JUnit is not needed? It is file IO, it can be slow.

test -z "$1" && test -n "$quiet" && return
shift
+
+ echo "$*" >> .junit/tout
Copy link
Member

Choose a reason for hiding this comment

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

Same in here.

esac
shift
+
+ echo "$*" >> .junit/tout
Copy link
Member

Choose a reason for hiding this comment

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

And here.

# This is a separate function because some tests use
# "return" to end a test_expect_success block early.
- eval </dev/null >&3 2>&4 "$*"
+ eval </dev/null > >(tee -a .junit/tout >&3) 2> >(tee -a .junit/terr >&4) "$*"
Copy link
Member

Choose a reason for hiding this comment

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

and here.

Copy link
Member

Choose a reason for hiding this comment

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

and in general.

@magik6k magik6k force-pushed the feat/sharness-junit branch 9 times, most recently from 059a80a to 087e114 Compare March 14, 2018 22:32
License: MIT
Signed-off-by: Łukasz Magiera <[email protected]>
License: MIT
Signed-off-by: Łukasz Magiera <[email protected]>
License: MIT
Signed-off-by: Łukasz Magiera <[email protected]>
@Kubuxu
Copy link
Member

Kubuxu commented Apr 11, 2018

@magik6k can we not increase the scope of this PR with benchmarking? Let's get it merged ASAP.

@magik6k
Copy link
Member Author

magik6k commented Apr 11, 2018

Good point. Extracted the benchmark part into #4932

@magik6k magik6k force-pushed the feat/sharness-junit branch 9 times, most recently from d867e18 to 7b6c449 Compare April 19, 2018 11:44
@Kubuxu
Copy link
Member

Kubuxu commented Apr 19, 2018

@magik6k can we resolve the Jenkins being red? I am ok with even disabling OSX and Windows on Jenkins for time being until we can resolve it.

The JUnit reporting is important enough to have it merged before that can be resolved.

@magik6k magik6k force-pushed the feat/sharness-junit branch 2 times, most recently from faceaf6 to 272718c Compare April 19, 2018 13:04
License: MIT
Signed-off-by: Łukasz Magiera <[email protected]>
@magik6k
Copy link
Member Author

magik6k commented Apr 19, 2018

It will be resolved by ipfs-inactive/jenkins#107, which i hope will be resolved soon

@Kubuxu
Copy link
Member

Kubuxu commented Apr 20, 2018

ipfs-inactive/jenkins#107 is resolved. RFM?

@Kubuxu Kubuxu added RFM and removed need/review Needs a review labels Apr 20, 2018
@whyrusleeping whyrusleeping merged commit 67a8cb0 into master Apr 20, 2018
@ghost ghost removed the status/in-progress In progress label Apr 20, 2018
@whyrusleeping whyrusleeping deleted the feat/sharness-junit branch April 20, 2018 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/test Testing work RFM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants