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.

Sign up for GitHub

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

Infrastructure: Use markdown to format PR comments containing regression test coverage reports #1562

Merged
merged 6 commits into from
Oct 22, 2020
Merged

Infrastructure: Use markdown to format PR comments containing regression test coverage reports #1562

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
85b8ebe
Switch the regression report to use markdown
Oct 9, 2020
9874e14
Change headings for clarify
Oct 22, 2020
bc788a5
Merge branch 'master' into markdown-regression-report
Oct 22, 2020
6c65e25
Fix grammar
Oct 22, 2020
ef6e15f
Grammar
Oct 22, 2020
4dc9956
Merge branch 'master' into markdown-regression-report
mcking65 Oct 22, 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
2 changes: 1 addition & 1 deletion .github/workflows/coverage-report.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ jobs:
github-token: ${{secrets.GITHUB_TOKEN}}
script: |
const fs = require('fs');
const commentBody = '```' + fs.readFileSync('coverage.log', 'utf8') + '```';
const commentBody = fs.readFileSync('coverage.log', 'utf8');
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the issue you're running into is that this change won't show up till this PR lands, because it is using pull_request_target

Copy link
Contributor

Choose a reason for hiding this comment

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

The code fence was just my hacky way of dealing with the whitespace collapsing on the original format, but your other changes should address that

if (commentBody.length === 0) {
return
Expand Down
25 changes: 13 additions & 12 deletions test/util/report.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,33 +215,33 @@ for (let example in exampleCoverage) {
let exampleName = example;

if (existingTestIds === missingTests) {
examplesWithNoTestsReport += exampleName + '\n';
examplesWithNoTestsReport += '- ' + exampleName + '\n';
examplesWithNoTests++;
}
else if (missingTests) {
examplesMissingSomeTestsReport += exampleName + ':\n';
examplesMissingSomeTestsReport += '- ' + exampleName + ':\n';

for (let testId of exampleCoverage[example].missingTests) {
examplesMissingSomeTestsReport += ' ' + testId + '\n';
examplesMissingSomeTestsReport += ' - ' + testId + '\n';
}

examplesMissingSomeTests += 1;
missingTestIds += missingTests;
}

if (missingKeys || missingAttrs) {
missingTestIdsReport += exampleName + '\n';
missingTestIdsReport += '- ' + exampleName + '\n';
if (missingKeys) {
missingTestIdsReport += ' "Keyboard Support" table(s):\n';
missingTestIdsReport += ' - "Keyboard Support" table(s):\n';
for (let row of exampleCoverage[example].missingKeys) {
missingTestIdsReport += ' ' + row + '\n';
missingTestIdsReport += ' - ' + row + '\n';
}
}

if (missingAttrs) {
missingTestIdsReport += ' "Attributes" table(s):\n';
missingTestIdsReport += ' - "Attributes" table(s):\n';
for (let row of exampleCoverage[example].missingAttrs) {
missingTestIdsReport += ' ' + row + '\n';
missingTestIdsReport += ' - ' + row + '\n';
}
}
}
Expand All @@ -260,14 +260,15 @@ fs.readdirSync(testsPath).forEach(function (testFile) {
}
});

console.log('\nExamples without any regression tests:\n');
console.log('\n## Regression test coverage:\n');
console.log('\n### Examples without any regression tests:\n');
Copy link
Contributor

@mcking65 mcking65 Oct 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Please make level 4 headings for all the headings. The GitHub comments themselves have level 3 heading, and we want these to be subheadings.

console.log(examplesWithNoTestsReport || 'None found.\n');
console.log('\nExamples missing some regression tests:\n');
console.log('\n### Examples missing some regression tests:\n');
console.log(examplesMissingSomeTestsReport || 'None found.\n');
console.log('\nExamples documentation table rows without data-test-ids:\n');
console.log('\n### Examples documentation table rows without data-test-ids:\n');
console.log(missingTestIdsReport || 'None found.\n');

console.log('SUMMARTY:\n');
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix spelling of summary

console.log('### SUMMARTY:\n');
console.log(' ' + exampleFiles.length + ' example pages found.');
Copy link
Contributor

Choose a reason for hiding this comment

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

The whitspace padding gets lost, so it can probably get dropped here and the lines below. Not sure if \t would work instead if the indent should be kept

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 white space padding is not super important, it was just ot make the console output more readable and less of a wall of text. In this context I think it's totally fine!

Copy link
Contributor

Choose a reason for hiding this comment

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

Random though, you could just make these 2 items a list

console.log(' ' + examplesWithNoTests + ' example pages have no regression tests.');
console.log(' ' + examplesMissingSomeTests + ' example pages are missing approximately ' +
Expand Down