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

JUnit support for meteor self-test test runner. #9014

Merged
merged 2 commits into from
Aug 16, 2017
Merged

Conversation

abernix
Copy link
Contributor

@abernix abernix commented Aug 14, 2017

This started as a proof-of-concept, but I think it's worth submitting for consideration.

In an effort to get more fine-grained information out of the meteor self-test tool (which is used for testing of Meteor releases during development) and to take advantage of additional features in CircleCI (and other test environments), this introduces a --junit=output.xml argument to the meteor self-test ... command.

This gains us a couple relatively functionally pleasing features, such as the ability to see failed tests without scrolling through all the results and CircleCI Insights on test timings.

One of the more important historical pieces of information we will gain with this is the timings of each individual test. Using this information, which is stored by CircleCI automatically in /.circleci-task-data/circle-test-results/ and also available through their API, we should be able to auto-balance tests using a partitioning algorithm and have our tests all run in as little time as possible. Though, I think after we gather this information for a week, we may be able to just manually re-balance the containers as well.

This utilizes the JUnit output format as it seems to be the most widely supported by CI environments – if I'm missing some common knowledge here, please do tell. A standardized, widely-supported, simple JSON format would be nice! Unfortunately, without that, this has the requirement of generating XML from the meteor self-test code. Given that the structure was relatively simple and I was hesitant to include an XML-building npm (we have xmlbuilder already as a Meteor package, used in Cordova, but it's not an individual isopacket). If we wanted to, I could change this to auto-install some xml-building package via a similar method as #8981, though that would require Meteor release-1.6.

@abernix
Copy link
Contributor Author

abernix commented Aug 14, 2017

For example! One of the containers failed on this PR, but here's the results: https://circleci.com/gh/meteor/meteor/5617.

Currently, I'm just dumping the whole test object in the event of a failure since it seemed mildly helpful in debugging some of the output parsing/wait-time issues, but we could probably refine it a bit.

@benjamn
Copy link
Contributor

benjamn commented Aug 14, 2017

If you really wanted to get fancy, you could use the xmlbuilder Meteor package!

Copy link
Contributor

@hwillson hwillson left a comment

Choose a reason for hiding this comment

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

Great idea @abernix - everything looks good to me - LGTM!

this.fileInfo[test.file].hasFailures = true;
test.failure = failure || true;
Copy link
Contributor

@benjamn benjamn Aug 15, 2017

Choose a reason for hiding this comment

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

I think this should be

notifyFailed(test, failure = true) {
  this.fileInfo[test.file].hasFailures = true;
  test.failure = failure;
}

If someone calls testList.notifyFailed(test, false), then test.failure = failure || true will set test.failure to true, which seems surprising?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the signature could be better for this. Perhaps notifyFailed(test, failureObject = null), and it could set a variable called test.failed = true (always) and test.failureObject = failureObject. I'll adjust tomorrow unless there is an objection.

`</testsuites>`,
].join('\n'),
'utf8',
);
Copy link
Contributor

Choose a reason for hiding this comment

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

I wasn't really serious about using xmlbuilder (via an isopacket) here, and I am not asking you to change this code, but this sure is a lot of raw string concatenation!

Copy link
Contributor Author

@abernix abernix Aug 15, 2017

Choose a reason for hiding this comment

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

I know. 🤕 I almost did the isopacket approach (and introduced a new one, since xmlbuilder is only otherwise built into cordova-support and I secretly envisioned that going away and didn't like the idea of adding another isopacket. Additionally, the xmlbuilder package used the xmlbuilder which was 7 major versions behind.

@benjamn benjamn added this to the Release 1.5.2 milestone Aug 16, 2017
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.

None yet

3 participants