-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Conversation
Aww, but that was my favorite part |
@magik6k @victorbjelkholm whats the blocker here? |
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 |
bb85024
to
cfe0d29
Compare
cfe0d29
to
e6cbe87
Compare
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. |
test/sharness/Rules.mk
Outdated
.PHONY: $(T_$(d)) | ||
|
||
$(d)/aggregate: $(T_$(d)) | ||
@echo "*** $@ ***" | ||
@(cd $(@D) && ./lib/test-aggregate-results.sh) | ||
@(cd $(@D) && ./lib/gen-junit-report.sh) |
There was a problem hiding this comment.
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.
test/sharness/Rules.mk
Outdated
.PHONY: $(T_$(d)) | ||
|
||
$(d)/aggregate: $(T_$(d)) | ||
@echo "*** $@ ***" | ||
@(cd $(@D) && ./lib/test-aggregate-results.sh) | ||
.PHONY: $(d)/aggregate | ||
|
||
$(d)/junit_xml: $(T_$(d)) |
There was a problem hiding this comment.
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.
test/sharness/Rules.mk
Outdated
@@ -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 |
There was a problem hiding this comment.
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() { |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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) "$*" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and in general.
059a80a
to
087e114
Compare
License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
934e5f0
to
826fcab
Compare
@magik6k can we not increase the scope of this PR with benchmarking? Let's get it merged ASAP. |
826fcab
to
6b3915b
Compare
Good point. Extracted the benchmark part into #4932 |
d867e18
to
7b6c449
Compare
@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. |
faceaf6
to
272718c
Compare
License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
272718c
to
fe62f77
Compare
It will be resolved by ipfs-inactive/jenkins#107, which i hope will be resolved soon |
ipfs-inactive/jenkins#107 is resolved. RFM? |
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