Details

Reviewers

deadalnix
jasonbcox

Group Reviewers

Restricted Project

Commits

rSTAGING6c6c08de32b2: Add an option to set the functional test suite name
rABC6c6c08de32b2: Add an option to set the functional test suite name

Summary

Teamcity is relying on the test suite name extracted from the JUnit XML report to display display the suite and its tests.
This test suite name is hardcoded, so Teamcity treats all the functional test runs as a single one, preventing from accessing the test details from the web UI.
This diff adds an option to test_runner.py to allow for setting a name for the test suite, and use it to reflect the intent of the runs.

Teamcity UI before this diff (the test_runner.py script is invoked twice on this build) Link to the example build

AllTestsOverviewBefore.PNG (213×548 px, 10 KB)

The UI show 110 tests instead of 220.

The tests appear only once, despite they are run twice.

Teamcity UI after this diff Link to the example build

AllTestsOverviewAfter.PNG (244×555 px, 11 KB)

The UI shows 220 tests as expected.

The tests appear twice, one for each run. Each test for each run can be accessed directly by clicking the next to the test name and select Show in Build Log

Test Plan

Look at the test list in the CI UI and check all the tests are present (220 at the moment).
Use the "Tests" tab to navigate through the tests and check the Show in Build Log link jumps to the related part of the build log.

Diff Detail

Repository

rABC Bitcoin ABC

Branch

test_runner_junit_test_suite_name_unique

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 8945
Build 15863: Default Diff Build & Tests
Build 15862: arc lint + arc unit

Event Timeline

Fabien created this revision.Jan 14 2020, 21:21

Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 14 2020, 21:21

Herald added a reviewer: deadalnix. · View Herald Transcript

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8945: Diff 15467.Jan 14 2020, 21:48

Testing if the id attribute is managed by teamcity.

Fabien planned changes to this revision.Jan 15 2020, 08:01

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing failed.

Snippet of first build failure:

Build 'Bitcoin-ABC / Diffs / Diff Testing' #7777, branch 'phabricator/diff/15472'
Started 2020-01-15 08:02:18 on 'highperf2' by 'Phabricator Staging (phabricator-staging)'
Finished 2020-01-15 08:06:54 with status FAILURE 'Failed to parse xml report (new); exit code 1 (Step: Command Line) (new)'

Harbormaster failed remote builds in B8949: Diff 15472!Jan 15 2020, 08:06

Make id an int

Build Bitcoin-ABC / Diffs / Diff Testing started.

Fabien planned changes to this revision.Jan 15 2020, 08:08

Build Bitcoin-ABC / Diffs / Diff Testing failed.

Snippet of first build failure:

Build 'Bitcoin-ABC / Diffs / Diff Testing' #7778, branch 'phabricator/diff/15473'
Started 2020-01-15 08:08:54 on 'highperf2' by 'Phabricator Staging (phabricator-staging)'
Finished 2020-01-15 08:11:26 with status FAILURE 'Failed to parse xml report (new); exit code 1 (Step: Command Line) (new)'

Harbormaster failed remote builds in B8950: Diff 15473!Jan 15 2020, 08:11

And now a string representing an int

Fabien planned changes to this revision.Jan 15 2020, 08:12

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8951: Diff 15474.Jan 15 2020, 08:17

Fabien edited the summary of this revision. (Show Details)Jan 15 2020, 12:54

Fabien retitled this revision from [WIP] Make the test suite name unique in the JUnit result file to Make the test suite name unique in the JUnit result file.Jan 15 2020, 13:12

After some tests, ready to review.

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8954: Diff 15477.Jan 15 2020, 13:19

I don't understand the problem. The fact that the test plan is not reproducible doesn't help. I can look all day long if I don't know what I'm looking for.

If the problem is that various tests needs to have a unique name, then why not use the name of the test rather than the temp directory?

This revision now requires changes to proceed.Jan 15 2020, 15:23

Fabien edited the summary of this revision. (Show Details)Jan 15 2020, 16:22

Fabien edited the summary of this revision. (Show Details)

Fabien edited the test plan for this revision. (Show Details)Jan 15 2020, 16:30

Fabien edited the summary of this revision. (Show Details)

Hopefully make the problem more clear with the use of screenshots.
Also improved the test plan a bit.

Fabien edited the summary of this revision. (Show Details)Jan 15 2020, 16:59

We had discussed offline about adding the commandline args to the junit name to make it obvious which test run it was. Any reason why you didn't include this? I think it drastically improves the usability of this fix.

This revision now requires changes to proceed.Jan 15 2020, 22:41

The reasoning behind not using the command line arguments is that I don't wanted to make the JUnit file a Teamcity only file. The name attribute is really designed to tell what the test suite being run is, and the arguments are more designed to fit a test suite property node imo.
Since the name needs to be unique, it seems to me that the temp dir is what better describes "what the test suite being run is", which is the closest to the initial name intent. You can read it as the name of "this instance of the bitcoin abc test suite".
Furthermore, it makes the name unique, which is not the case if you use the command line parameters.

Long story:
There are various testsuite attributes in a JUnit test report XML file, and even one, id, designed to solve our problem (multiple runs of the same test suite).
After a few tests and reading the doc (in fact, Teamcity parser code since there is no doc) it appears that Teamcity doesn't interpret any of the attributes that could have been helpful to solve our issue, including the id one.
The only thing that can be done to make Teamcity understand that there are multiple runs of the same test suite is to have a different test suite name for the different runs. It's not really a bug because there is not really a specification of the format, and as such every single tool use a slightly different schema. Since we can't do the right thing, at least we can try do be as close as possible from the original intent.

jasonbcox accepted this revision.Jan 16 2020, 21:58

Looking at the screenshot I understand better. You get 2 runs due to pre and post activation. This is what you want to indicate in the name of the test suite - as well as other parameters if they come to be more in the future.

This revision now requires changes to proceed.Jan 17 2020, 01:47

Fabien planned changes to this revision.Jan 20 2020, 13:10

Fabien edited the summary of this revision. (Show Details)Jan 20 2020, 13:49

Fabien retitled this revision from Make the test suite name unique in the JUnit result file to Add an option to set the functional test suite name.

Add an option to set the name.

Fabien planned changes to this revision.Jan 20 2020, 13:54

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B9074: Diff 15684.Jan 20 2020, 13:59

Fabien requested review of this revision.Jan 20 2020, 14:05

Fabien edited the summary of this revision. (Show Details)

Diff updated to address feedback.
The method has changed since the last revision: I added a new option that allow the caller to set a meaningful test suite name and use it in the JUnit output.

deadalnix requested changes to this revision.Jan 20 2020, 22:53

deadalnix added inline comments.

test/CMakeLists.txt
73 ↗	(On Diff #15684)	This probably shouldn't have a different name, because the tests are the same and should be tracked by teamcity as the same. (historical data on time, flakiness, etc...).
test/functional/test_runner.py
180 ↗	(On Diff #15684)	This should probably be computed by the test runner from the arguments which are known to produce different test suites. My understanding is that you want to keep the same test under the same test suite name so that teamcity consider it the same.

This revision now requires changes to proceed.Jan 20 2020, 22:53

Fabien planned changes to this revision.Jan 21 2020, 10:20

Fabien added inline comments.

test/CMakeLists.txt
73 ↗	(On Diff #15684)	Good point, that need to be fixed.
test/functional/test_runner.py
180 ↗	(On Diff #15684)	This was the initial solution, but I think adding this option is a better choice for a few reasons: The test_runner.py has no other context information that its arguments. This is good enough if the only change is from the activation flag, but it hardly stands if other args/environment variables comes into play. On the other end the caller knows what the test suite is intended for. The first example that comes to my mind is if you run the test suite on different OSes, you might want to differentiate the failure tracking. Inspecting the arguments as the potential to add more maintenance. The way the arguments are handled makes it a bit hacky, since some are passed through to the framework (this is in particular the case of the activation flag). Since these options can impact the test suite, the test_runner.py would have to search for the arguments it doesn't use and interpret them.