-
Notifications
You must be signed in to change notification settings - Fork 57
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
Add code coverage upload command #1520
base: master
Are you sure you want to change the base?
Add code coverage upload command #1520
Conversation
Datadog ReportBranch report: ✅ 0 Failed, 244 Passed, 0 Skipped, 1m 37.24s Total duration (2m 17.58s time saved) |
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.
Approved.
I see [//]: <> (TODO: Add link to the documentation page)
in the doc. Please update this before publishing.
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.
looks mostly good! Left minor comments 😄
src/commands/coverage/README.md
Outdated
``` | ||
|
||
- `--path` is the directory or file path in which the code coverage reports are located. If you pass a folder, the CLI will look for all `.xml` files in it. This argument can be provided multiple times with different values | ||
- `--flush` (default: `false`): you may pass `--flush=1` or `--flush=true` to signal that you have uploaded all the coverage reports for the current commit/PR. This will trigger the coverage reports to be processed and the results to displayed in the UI. |
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.
just curious, what happens if the user forgets to pass this?
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.
The reports will be uploaded to Datadog, but the coverage metric will not be produced. This is needed for the cases when there are multiple independent CI jobs producing their own coverage reports - in such scenario the flush should only happen once all of the reports from all of the jobs have been uploaded
src/commands/coverage/README.md
Outdated
- The resulting dictionary will be merged with whatever is in the `DD_MEASURES` environment variable. If a `key` appears both in `--measures` and `DD_MEASURES`, whatever value is in `DD_MEASURES` will take precedence. | ||
- `--dry-run` (default: `false`): it will run the command without the final upload step. All other checks are performed. | ||
- `--skip-git-metadata-upload` (default: `true`): if you want to upload git metadata, you may pass `--skip-git-metadata-upload=0` or `--skip-git-metadata-upload=false`. | ||
- `--git-repository-url` is a string with the repository URL to retrieve git metadata from. If this is missing, the URL is retrieved from the local git repository. |
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.
this has confused users in the past. Do we need it? I think this should've only been in the git metadata upload command
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.
This is indeed optional. I don't know how commonly this is used. Removed it for now
src/commands/coverage/upload.ts
Outdated
const pathsByFormat: {format: string; paths: string[]}[] = [] | ||
|
||
for (const codeCoverageReportPath of uniqueFiles) { | ||
const format: string | undefined = (await detectFormat(codeCoverageReportPath)) || this.format | ||
const validationErrorMessage = await validateCoverageReport(codeCoverageReportPath, format, this.format) | ||
if (validationErrorMessage) { | ||
this.context.stdout.write(renderInvalidFile(codeCoverageReportPath, validationErrorMessage)) | ||
} else { | ||
let formatEntry = pathsByFormat.find((entry) => entry.format === format) | ||
if (!formatEntry) { | ||
formatEntry = {format: format as string, paths: []} | ||
pathsByFormat.push(formatEntry) | ||
} | ||
formatEntry.paths.push(codeCoverageReportPath) | ||
} | ||
} | ||
|
||
return pathsByFormat |
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.
I think pathsByFormat
could actually be a dictionary more simply 😄 .
❓ Also: what's this.format
?
const pathsByFormat: {[key:string]: string[]} = {}
for (const codeCoverageReportPath of uniqueFiles) {
const format = detectFormat(codeCoverageReportPath)
const validationErrorMessage = validateCoverageReport(codeCoverageReportPath, format)
if (validationErrorMessage) {
this.context.stdout.write(renderInvalidFile(codeCoverageReportPath, validationErrorMessage))
} else {
const paths = pathsByFormat[format]
if (!paths) {
pathsByFormat[format] = [codeCoverageReportPath]
} else {
pathsByFormat[format].push(codeCoverageReportPath)
}
}
}
return pathsByFormat
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.
Changed it to be a dictionary.
this.format
allows the users to specify format manually in case our logic cannot detect it automatically
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.
Looks good on my end, thanks for handling the comments!
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.
👏 Great job! Left some minor comments
Datadog ReportBranch report: ✅ 0 Failed, 9116 Passed, 0 Skipped, 4m 30.8s Total Time |
What and why?
Adds a new
coverage upload
command that sends code coverage reports to Datadog.See EVP usage in Code Coverage product prototype RFC for details
SDTEST-1183
Review checklist