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

[Security Solution][Serverless] Github tickets / notifications #197265

Merged
merged 15 commits into from
Nov 7, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ run(

let branch: string = '';
let pipeline: string = '';
let prependTitle: string = '';
if (updateGithub) {
let isPr = false;

Expand All @@ -52,6 +53,7 @@ run(
pipeline = process.env.BUILDKITE_PIPELINE_SLUG || '';
isPr = process.env.BUILDKITE_PULL_REQUEST === 'true';
updateGithub = process.env.REPORT_FAILED_TESTS_TO_GITHUB === 'true';
prependTitle = process.env.PREPEND_FAILURE_TITLE || '';
} else {
// JOB_NAME is formatted as `elastic+kibana+7.x` in some places and `elastic+kibana+7.x/JOB=kibana-intake,node=immutable` in others
const jobNameSplit = (process.env.JOB_NAME || '').split(/\+|\//);
Expand Down Expand Up @@ -154,7 +156,14 @@ run(
continue;
}

const newIssue = await createFailureIssue(buildUrl, failure, githubApi, branch, pipeline);
const newIssue = await createFailureIssue(
buildUrl,
failure,
githubApi,
branch,
pipeline,
prependTitle
);
existingIssues.addNewlyCreated(failure, newIssue);
pushMessage('Test has not failed recently on tracked branches');
if (updateGithub) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,53 @@ describe('createFailureIssue()', () => {
}
`);
});

it('creates new github issue with title prepended', async () => {
const api = new GithubApi();

await createFailureIssue(
'https://build-url',
{
classname: 'some.classname',
failure: 'this is the failure text',
name: 'test name',
time: '2018-01-01T01:00:00Z',
likelyIrrelevant: false,
},
api,
'main',
'kibana-on-merge',
'[MKI][QA]'
);

expect(api.createIssue).toMatchInlineSnapshot(`
[MockFunction] {
"calls": Array [
Array [
"Failing test: [MKI][QA] some.classname - test name",
"A test failed on a tracked branch
\`\`\`
this is the failure text
\`\`\`
First failure: [kibana-on-merge - main](https://build-url)
<!-- kibanaCiData = {\\"failed-test\\":{\\"test.class\\":\\"some.classname\\",\\"test.name\\":\\"test name\\",\\"test.failCount\\":1}} -->",
Array [
"failed-test",
],
],
],
"results": Array [
Object {
"type": "return",
"value": undefined,
},
],
}
`);
});
});

describe('updateFailureIssue()', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,15 @@ export async function createFailureIssue(
failure: TestFailure,
api: GithubApi,
branch: string,
pipeline: string
pipeline: string,
prependTitle: string = ''
) {
const title = `Failing test: ${failure.classname} - ${failure.name}`;
// PrependTitle is introduced to provide some clarity by prepending the failing test title
// in order to give the whole info in the title according to each team's preference.
const title =
prependTitle && prependTitle.trim() !== ''
? `Failing test: ${prependTitle} ${failure.classname} - ${failure.name}`
: `Failing test: ${failure.classname} - ${failure.name}`;

// Github API body length maximum is 65536 characters
// Let's keep consistency with Mocha output that is truncated to 8192 characters
Expand Down
38 changes: 38 additions & 0 deletions x-pack/plugins/security_solution/scripts/junit_transformer/lib.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,20 @@ import { PathReporter } from 'io-ts/lib/PathReporter';
import globby from 'globby';
import del from 'del';

// Function to remove specific fields from an XML object in order to
// compare them as strings.
// eslint-disable-next-line @typescript-eslint/no-explicit-any
function removeFields(obj: any, fieldsToRemove: string[]): any {
Copy link
Member

Choose a reason for hiding this comment

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

any reason for not having an explicit test for this function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Checking over the tests of the whole package junit_transformer it looks like it is not expicitly tested as it relates to test code. I guess its not needed to try to keep a non existing test coverage. Also this part of code is tested by the generic test updates the file in place, applying the expected transformation as all of the rest of the code.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I'd prefer an explicit test, but you sound confident enough in your response to me.
Thanks.

Oh, one last thing, I was not referring to coverage, but to my own personal nits about testing functions explicitly.
From your comment, it sounds like removeFields is only tested implicitly.
As this lib.ts file is not actually owned by my team, no worries.

for (const key in obj) {
if (fieldsToRemove.includes(key)) {
delete obj[key];
Copy link
Member

Choose a reason for hiding this comment

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

NIT: I believe it would be greate if we can avoid the modification of the original object by making a copy first and doing the modifications there with the aim of avoid side effects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But we do not change the actual file at any point. We just getting a parsed string out of it and then we remove the fields for future use.

} else if (typeof obj[key] === 'object') {
obj[key] = removeFields(obj[key], fieldsToRemove); // Recursively remove fields
}
}
return obj;
}

/**
* Updates the `name` and `classname` attributes of each testcase.
* `name` will have the value of `classname` appended to it. This makes sense because they each contain part of the bdd spec.
Expand Down Expand Up @@ -174,6 +188,10 @@ export async function command({ flags, log }: CommandArgs) {
throw createFlagError('please provide a single --reportName flag');
}

// Going to be used in order to store results as string in order to compare
// and remove duplicated reports.
const xmlResultFiles: string[] = [];

for (const path of await globby(flags.pathPattern)) {
// Read the file
const source: string = await fs.readFile(path, 'utf8');
Expand Down Expand Up @@ -242,6 +260,26 @@ ${boilerplate}
rootDirectory: flags.rootDirectory,
});

// We need to check if a XML Junit report is duplicate
// Only if we remove the time and timestamp and the rest of the
// report as a string is completely identical.
const fieldsToRemove = ['time', 'timestamp'];
const tempReport = await parseStringPromise(reportString);
const truncatedReport = removeFields(tempReport, fieldsToRemove);

// Rebuild the XML to compare (optional, if you want to compare XML strings)
const builder = new Builder();
const rebuildComparableReport = builder.buildObject(truncatedReport);

// Compare the cleaned and rebuilt XML objects or strings
if (xmlResultFiles.includes(rebuildComparableReport)) {
// If the report is a duplicate, we need to remove the file
// in order to be excluded from the uploaded results.
await del(path, { force: true });
continue;
}
xmlResultFiles.push(rebuildComparableReport);

// If the writeInPlace flag was passed, overwrite the original file, otherwise log the output to stdout
if (flags.writeInPlace) {
log.info(`Wrote transformed junit report to ${path}`);
Expand Down