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

Add support for NUnit3 XML reports from UITest #572

Merged

Conversation

owenniblock
Copy link
Member

Motivation

NUnit3 test runs against the latest UITest fail when they attempt to merge the output XML (this is because of a bug in the merging on the pipeline but also because of the code here which needed updating to match).

This PR adds support for the new NUnit3 report format.

Caveat

Looking at the existing code, I suspect that there are some issues merging result states between test runs (both in NUnit2 and NUnit3) - I have not had time to investigate this thoroughly but I believe it would manifest if running on several devices and only a subset of the devices fail.

Example CLI output:

Current test status: Running on 1 device (3 / 4 completed, 0 pending) - Report 50% done

Current test status: Tests completed. Processing data.

Current test status: Tests completed. Processing data.

Current test status: Tests completed. Processing data.

Current test status: Done!
Total scenarios: 4
4 passed
0 failed
Total steps: 6


Downloaded artifacts to /Users/owenniblock/tmp/nunit_xml_zip.zip

@maxxkrakoa - I'm not sure who's best to review this PR. Normally I'd ask Simon - let me know!

const testSuitesParent: Element = this.collectAllElements(xml1.documentElement, "test-run")[0];
const testSuites: Element[] = this.collectChildren(xml2.documentElement, "test-suite");

testSuites.forEach((child: Element) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can use the spread operator here to make the code simpler to read:

testSuitesParent.append(...testSuites)

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice!

Copy link
Member Author

Choose a reason for hiding this comment

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

I couldn't get this to work:

.appendChild isn't compatible with ... because it expects at least 1 child element this error gets raised

.append appears to not work for an Element which is confusing. Unless you're super keen for me to do this I suggest we don't invest too much time in it.

}

isNUnit3(xml: Document): boolean {
const testResults: Element[] = this.collectAllElements(xml.documentElement, "test-results");
Copy link
Contributor

Choose a reason for hiding this comment

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

this is more of a not nunit2 test. Perhaps its better to test for precense of "test-run" instead of absence of "test-results"

Copy link
Member Author

@owenniblock owenniblock Apr 29, 2019

Choose a reason for hiding this comment

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

I've done it like this pending NUnit4 coming out so that it's easier to convert to:

getNUnitVersion: int

Or something similar. Although it might not be the case, I figured NUnit4 is more likely to follow similar report format the NUnit3 - at which point we may need to dig in to the engine-version attribute on the test-results node.

src/commands/test/lib/nunit-xml-util.ts Outdated Show resolved Hide resolved
@owenniblock
Copy link
Member Author

@john7doe - I've not done 2 of your suggestions - let's discuss tomorrow if you think they're important enough to do!

@owenniblock
Copy link
Member Author

Merging despite CI build error - will handle that separately.

@owenniblock owenniblock merged commit af594fe into microsoft:master Apr 30, 2019
@owenniblock owenniblock deleted the feature/nunit3-xml-report-support branch April 30, 2019 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants