Skip to content
This repository has been archived by the owner on Sep 16, 2021. It is now read-only.

feat: endpoint to post xunit test data #254

Merged
merged 22 commits into from
Aug 21, 2020
Merged

feat: endpoint to post xunit test data #254

merged 22 commits into from
Aug 21, 2020

Conversation

cedpeters
Copy link
Contributor

No description provided.

@google-cla google-cla bot added the cla: yes label Aug 18, 2020
@cedpeters cedpeters requested a review from bcoe August 18, 2020 22:51
Copy link
Contributor

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

Left a few nits, this is looking really good though.

packages/api/lib/xunit-parser.js Outdated Show resolved Hide resolved
packages/api/lib/xunit-parser.js Outdated Show resolved Hide resolved
packages/api/src/post-build.js Show resolved Hide resolved
packages/api/src/post-build.js Outdated Show resolved Hide resolved
@cedpeters cedpeters marked this pull request as ready for review August 20, 2020 23:09
packages/api/lib/xunit-parser.js Show resolved Hide resolved
packages/api/src/post-build.js Outdated Show resolved Hide resolved
@@ -96,7 +101,7 @@ class PostBuildHandler {
if (typeof x.ok !== 'boolean' || !x.id || !x.name) {
throw new InvalidParameterError('Missing All Test Case Info');
}
const testcase = new TestCaseRun(x.ok ? 'ok' : 'not ok', x.id, x.name);
const testcase = new TestCaseRun(x.ok ? 'ok' : 'not ok', x.name);

Choose a reason for hiding this comment

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

You seem to be using "ok" and "not ok" strings in multiple places -- might be helpful to save them as constants and reuse the constant

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 this is actually a quirk of parsing tap files, I don't think I can change it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm okay with leaving this for now ... I find the fact that we create a string ok, not ok weird, as it's immediately turned into a boolean when we store it, but we already had this behavior.

tldr; seems like logic worth cleaning up.

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 did actually go through and start to clean it up, but that constructor is called in a number of different places and it was getting a bit messy. I'll make an issue for this to be addressed at some point, though.

packages/api/lib/xunit-parser.js Outdated Show resolved Hide resolved
packages/api/lib/xunit-parser.js Show resolved Hide resolved
packages/api/test/post-build-test.js Outdated Show resolved Hide resolved
@cedpeters cedpeters merged commit 83ca7a0 into master Aug 21, 2020
let log = (failure === undefined) ? error._text : failure._text;
// Java puts its test logs in a CDATA element.
if (log === undefined) {
log = failure._cdata;
Copy link

Choose a reason for hiding this comment

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

Need to update this to also get error._cdata. See googleapis/repo-automation-bots#873, which simplifies the error vs failure logic a bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh okay, thank you! Added this to my new draft PR: #259


// endpoint expects the the required buildinfo to be in req.body.metadata to already exist and be properly formatted.
// required keys in the req.body.metadata are the inputs for addBuild in src/add-build.js
this.app.post('/api/build/xml', async (req, res, next) => {
Copy link

Choose a reason for hiding this comment

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

How do we get which repo or build this XML is for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That will be sent in the metadata. I wrote a rough draft of how the data will be sent, if we do use buildcop: https://github.com/cedpeters/repo-automation-bots/pull/1/files

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants