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
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
4c857e7
initial xunit-parser work
cedpeters Aug 18, 2020
7b224b5
get rid of pass and fail arrays; just have one array of tests
cedpeters Aug 18, 2020
2da2708
xunit-parser returns an array of testrun objects
cedpeters Aug 18, 2020
4053c91
endpoint code almost done through tests
cedpeters Aug 18, 2020
b4a276b
Merge branch 'master' into endpoint
cedpeters Aug 18, 2020
daa47e6
add second test which checks values in tests
cedpeters Aug 18, 2020
b9b1641
Merge branch 'endpoint' of github.com:GoogleCloudPlatform/flaky-servi…
cedpeters Aug 18, 2020
681c37d
test parsing failed test, name test full path minus github link
cedpeters Aug 18, 2020
b949655
linting
cedpeters Aug 18, 2020
5a07801
remove count var from testrun.js
cedpeters Aug 19, 2020
70fd1c4
const to var
cedpeters Aug 19, 2020
9631fe2
stub out the build parsing
cedpeters Aug 20, 2020
caf8468
two tests for new post-build endpoint
cedpeters Aug 20, 2020
6c19faa
Merge branch 'master' into endpoint
cedpeters Aug 20, 2020
1530917
delete commented out code
cedpeters Aug 20, 2020
376e68b
Merge branch 'endpoint' of github.com:GoogleCloudPlatform/flaky-servi…
cedpeters Aug 20, 2020
694afe6
one last commented out line deleted
cedpeters Aug 20, 2020
8c30ded
switch test name parser to use regex
cedpeters Aug 20, 2020
def620c
split line into shorter ones
cedpeters Aug 21, 2020
469d254
add comments
cedpeters Aug 21, 2020
9203c6e
replace stubs array with sinon.sandbox
cedpeters Aug 21, 2020
d952a42
refactor add-build so that it can more easily be tested
cedpeters Aug 21, 2020
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions packages/api/lib/testrun.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,15 @@
const firebaseEncode = require('./firebase-encode');

class TestCaseRun {
constructor (okMessage, number, name) {
constructor (okMessage, name) {
this.successful = (okMessage === 'ok');
this.number = number;
this.name = name;
this.encodedName = firebaseEncode(this.name);
this.failureMessage = 'Successful';
}

display () {
return this.number + ', ' + this.name + ', ' + this.time + ', ' + (this.successful ? '1' : '0') + ', ' + this.failureMessage;
return this.name + ', ' + this.time + ', ' + (this.successful ? '1' : '0') + ', ' + this.failureMessage;
}
}

Expand Down
92 changes: 92 additions & 0 deletions packages/api/lib/xunit-parser.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
// Copyright 2020 Google LLC
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// https://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

const xmljs = require('xml-js');
const TestCaseRun = require('../lib/testrun');

class Parser {
parse (xmlString) {
const obj = xmljs.xml2js(xmlString, { compact: true });
const tests = [];
// Python doesn't always have a top-level testsuites element.
let testsuites = obj.testsuite;
if (testsuites === undefined) {
testsuites = obj.testsuites.testsuite;
}
if (testsuites === undefined) {
return tests;
}
// If there is only one test suite, put it into an array to make it iterable.
if (!Array.isArray(testsuites)) {
testsuites = [testsuites];
}
for (const suite of testsuites) {
// Ruby doesn't always have _attributes.
let testsuiteName = suite._attributes ? suite._attributes.name : undefined;

// Get rid of github.com/orgName/repoName/
cedpeters marked this conversation as resolved.
Show resolved Hide resolved
testsuiteName = this.trim(testsuiteName);

let testcases = suite.testcase;
// If there were no tests in the package, continue.
if (testcases === undefined) {
continue;
}
// If there is only one test case, put it into an array to make it iterable.
if (!Array.isArray(testcases)) {
testcases = [testcases];
}

for (const testcase of testcases) {
// Ignore skipped tests. They didn't pass and they didn't fail.
if (testcase.skipped !== undefined) {
continue;
}

const failure = testcase.failure;
const error = testcase.error;

const testCaseRun = new TestCaseRun((failure === undefined && error === undefined) ? 'ok' : 'not ok', (testsuiteName !== '') ? testsuiteName + '/' + testcase._attributes.name : testcase._attributes.name);
cedpeters marked this conversation as resolved.
Show resolved Hide resolved

if (!testCaseRun.successful) {
// Here we must have a failure or an error.
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

}

testCaseRun.failureMessage = log;
}

tests.push(testCaseRun);
}
}
return tests;
}

trim (url) {
cedpeters marked this conversation as resolved.
Show resolved Hide resolved
const updated = url.replace(/(.)*github.com\/[a-zA-Z-]*\/[a-zA-Z-]*/g, '');
if (updated.length > 1) return updated.substring(1); // Get rid of starting `/`
return updated;
}

// IMPORTANT: All values that will be used as keys in Firestore must be escaped with the firestoreEncode function
cleanXunitBuildInfo (metadata) {
return {};
}
}

const parserHandler = new Parser();
module.exports = parserHandler;
3 changes: 2 additions & 1 deletion packages/api/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@
"express-session": "^1.17.1",
"moment": "^2.27.0",
"tap-parser": "^10.0.1",
"uuid": "^8.2.0"
"uuid": "^8.2.0",
"xml-js": "^1.6.11"
},
"devDependencies": {
"c8": "^7.1.2",
Expand Down
37 changes: 30 additions & 7 deletions packages/api/src/post-build.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@

const addBuild = require('../src/add-build');
const TestCaseRun = require('../lib/testrun');
var Parser = require('tap-parser');
const TapParser = require('tap-parser');
const xunitParser = require('../lib/xunit-parser');
const Readable = require('stream').Readable;
const firebaseEncode = require('../lib/firebase-encode');
const { InvalidParameterError, UnauthorizedError, handleError } = require('../lib/errors');
Expand All @@ -31,6 +32,10 @@ class PostBuildHandler {
this.client = client;
}

static async addBuild (testCases, buildInfo, client, collectionName) {
await addBuild(testCases, buildInfo, client, collectionName);
}
cedpeters marked this conversation as resolved.
Show resolved Hide resolved

static parseEnvironment (metadata) {
var envData = {
os: metadata.os.os || 'Not specified',
Expand Down Expand Up @@ -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.


// wrap failure message generation in try so still works if ids arent sequential
try {
Expand All @@ -121,7 +126,7 @@ class PostBuildHandler {
switch (fileType) {
case 'TAP': {
var data = [];
var p = new Parser();
var p = new TapParser();
cedpeters marked this conversation as resolved.
Show resolved Hide resolved

p.on('result', function (assert) {
data.push(assert);
Expand Down Expand Up @@ -169,7 +174,7 @@ class PostBuildHandler {
}

// IMPORTANT: All values that will be used as keys in Firestore must be escaped with the firestoreEncode function
static cleanBuildInfo (metadata) {
static cleanTapBuildInfo (metadata) {
const timestampNumb = Date.parse(metadata.timestamp);
const timestamp = isNaN(timestampNumb) ? new Date() : new Date(timestampNumb);

Expand Down Expand Up @@ -232,7 +237,7 @@ class PostBuildHandler {
throw new UnauthorizedError('Flaky does not store tests for private repos');
}

await addBuild(PostBuildHandler.removeDuplicateTestCases(testCases), buildInfo, this.client, global.headCollection);
await PostBuildHandler.addBuild(PostBuildHandler.removeDuplicateTestCases(testCases), buildInfo, this.client, global.headCollection);
res.send({ message: 'successfully added build' });
} catch (err) {
handleError(res, err);
Expand All @@ -247,7 +252,7 @@ class PostBuildHandler {
throw new UnauthorizedError('Flaky does not store tests for private repos');
}

const buildInfo = PostBuildHandler.cleanBuildInfo(req.body.metadata); // Different line. The metadata object is the same as addbuild, already validated
const buildInfo = PostBuildHandler.cleanTapBuildInfo(req.body.metadata); // Different line. The metadata object is the same as addbuild, already validated

req.body.data = await PostBuildHandler.flattenTap(req.body.data);
const parsedRaw = await PostBuildHandler.parseRawOutput(req.body.data, req.body.type);
Expand All @@ -258,7 +263,25 @@ class PostBuildHandler {
throw new UnauthorizedError('Must have valid Github Token to post build');
}

await addBuild(PostBuildHandler.removeDuplicateTestCases(testCases), buildInfo, this.client, global.headCollection);
await PostBuildHandler.addBuild(PostBuildHandler.removeDuplicateTestCases(testCases), buildInfo, this.client, global.headCollection);
res.send({ message: 'successfully added build' });
} catch (err) {
handleError(res, err);
}
});

// 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

try {
if (req.headers.authorization !== process.env.PRIVATE_POSTING_TOKEN) {
throw new UnauthorizedError('Invalid Secret. Only Google Employees may use this endpoint.');
}

const testCases = xunitParser.parse(req.body.data);
const buildInfo = xunitParser.cleanXunitBuildInfo(req.body.metadata);

await PostBuildHandler.addBuild(PostBuildHandler.removeDuplicateTestCases(testCases), buildInfo, this.client, global.headCollection);
res.send({ message: 'successfully added build' });
} catch (err) {
handleError(res, err);
Expand Down
22 changes: 11 additions & 11 deletions packages/api/test/addbuild-getbuild.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,10 @@ const buildInfo = [
},
timestamp: new Date('01/01/2000'),
testCases: [
new TestCaseRun('ok', 1, 'a/1'),
new TestCaseRun('not ok', 2, 'a/2'),
new TestCaseRun('ok', 3, 'a/3'),
new TestCaseRun('not ok', 4, 'a/4')
new TestCaseRun('ok', 'a/1'),
new TestCaseRun('not ok', 'a/2'),
new TestCaseRun('ok', 'a/3'),
new TestCaseRun('not ok', 'a/4')
],
description: 'nodejs repository',
buildmessage: 'Workflow - 1'
Expand All @@ -70,8 +70,8 @@ const buildInfo = [
},
timestamp: new Date('01/01/2001'),
testCases: [
new TestCaseRun('ok', 1, 'a/1'),
new TestCaseRun('ok', 2, 'a/2') // this test is now passing
new TestCaseRun('ok', 'a/1'),
new TestCaseRun('ok', 'a/2') // this test is now passing
],
description: 'nodejs repository',
buildmessage: 'Workflow - 2'
Expand All @@ -91,8 +91,8 @@ const buildInfo = [
},
timestamp: new Date('01/01/2002'),
testCases: [
new TestCaseRun('not ok', 1, 'a/5'),
new TestCaseRun('not ok', 2, 'a/2') // this test is now failing
new TestCaseRun('not ok', 'a/5'),
new TestCaseRun('not ok', 'a/2') // this test is now failing
],
description: 'None',
buildmessage: 'Workflow - 1'
Expand All @@ -102,7 +102,7 @@ const buildInfo = [
buildInfo[2].testCases[0].failureMessage = 'Error message';
buildInfo[2].testCases[1].failureMessage = 'Error message';

describe.only('Add-Build', () => {
describe('Add-Build', () => {
before(async () => {
global.headCollection = 'testing/' + TESTING_COLLECTION_BASE + uuidv4() + '/repos'; // random collection name for concurrent testing
});
Expand Down Expand Up @@ -434,8 +434,8 @@ describe.only('Add-Build', () => {
},
timestamp: new Date('01/01/2004'),
testCases: [
new TestCaseRun('not ok', 1, 'a/5'),
new TestCaseRun('not ok', 2, 'a/2') // this test is now passing
new TestCaseRun('not ok', 'a/5'),
new TestCaseRun('not ok', 'a/2') // this test is now passing
],
description: 'nodejs repository',
buildmessage: 'Workflow - 42'
Expand Down
2 changes: 2 additions & 0 deletions packages/api/test/fixtures/empty_results.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
<?xml version="1.0" encoding="UTF-8"?>
<testsuites></testsuites>
29 changes: 29 additions & 0 deletions packages/api/test/fixtures/go_failure_group.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
<?xml version="1.0" encoding="UTF-8"?>
<testsuites>
<testsuite tests="18" failures="18" time="4.364" name="github.com/GoogleCloudPlatform/golang-samples/bigquery/snippets/querying">
<properties>
<property name="go.version" value="go1.13.1"></property>
</properties>
<testcase classname="querying" name="TestQueries" time="0.000">
<failure message="Failed" type=""></failure>
</testcase>
<testcase classname="querying" name="TestQueries/group" time="0.000">
<failure message="Failed" type=""></failure>
</testcase>
<testcase classname="querying" name="TestQueries/group/queryBasic" time="0.000">
<failure message="Failed" type=""></failure>
</testcase>
<testcase classname="querying" name="TestQueries/group/queryBatch" time="0.000">
<failure message="Failed" type="">panic: runtime error: invalid memory address or nil pointer dereference&#xA;/usr/local/go/src/testing/testing.go:874 +0x3a3&#xA;/usr/local/go/src/runtime/panic.go:679 +0x1b2&#xA;/go/pkg/mod/cloud.google.com/go/bigquery@v1.3.0/iterator.go:106 +0x37&#xA;/tmpfs/src/github/golang-samples/bigquery/snippets/querying/bigquery_query_legacy_large_results.go:59 +0x419&#xA;/tmpfs/src/github/golang-samples/bigquery/snippets/querying/integration_test.go:90 +0xa5&#xA;/usr/local/go/src/testing/testing.go:909 +0xc9&#xA;/usr/local/go/src/testing/testing.go:960 +0x350</failure>
</testcase>
<testcase classname="querying" name="TestQueries/group/queryDisableCache" time="0.000">
<failure message="Failed" type=""></failure>
</testcase>
<testcase classname="querying" name="TestQueries/group/queryDryRun" time="0.000">
<failure message="Failed" type=""></failure>
</testcase>
<testcase classname="querying" name="TestQueries/group/queryLegacy" time="0.000">
<failure message="Failed" type=""></failure>
</testcase>
</testsuite>
</testsuites>
17 changes: 17 additions & 0 deletions packages/api/test/fixtures/go_skip.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
<?xml version="1.0" encoding="UTF-8"?>
<testsuites>
<testsuite tests="3" failures="0" time="27.297" name="github.com/GoogleCloudPlatform/golang-samples">
<properties>
<property name="go.version" value="go1.11.13"></property>
</properties>
<testcase classname="golang-samples" name="TestLicense" time="0.120"></testcase>
</testsuite>
<testsuite tests="2" failures="0" time="0.526" name="github.com/GoogleCloudPlatform/golang-samples/securitycenter/notifications">
<testcase classname="notifications" name="TestGetNotificationConfig" time="0.000">
<skipped message="notifications_test.go:162: https://github.com/GoogleCloudPlatform/golang-samples/issues/1354"></skipped>
</testcase>
<testcase classname="notifications" name="TestListNotificationConfigs" time="0.000">
<skipped message="notifications_test.go:186: https://github.com/GoogleCloudPlatform/golang-samples/issues/1355"></skipped>
</testcase>
</testsuite>
</testsuites>
Loading