-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Changes from 13 commits
e98b659
32a30d3
2b795e8
525c0bb
cee6463
e36fbfa
d4346f1
0bb0af7
8488140
bdfa5f4
908e168
5d17f9a
ffcc76c
60a4e7b
a4c08b9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 { | ||
for (const key in obj) { | ||
if (fieldsToRemove.includes(key)) { | ||
delete obj[key]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
@@ -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'); | ||
|
@@ -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}`); | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 testupdates the file in place, applying the expected transformation
as all of the rest of the code.There was a problem hiding this comment.
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.