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

Update coverage report script to process new content tree and put output into about (Issue 2418 task 9) #2451

Merged
merged 11 commits into from
Oct 27, 2022

Conversation

alflennik
Copy link
Contributor

@alflennik alflennik commented Aug 29, 2022

See #2418 for more information about the task.

@mcking65 mcking65 changed the title Implement 2418 task 9 Update coverage report script to process new content tree and put output into about (Issue 2418 task 9) Sep 4, 2022
Copy link
Contributor

@mcking65 mcking65 left a comment

Choose a reason for hiding this comment

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

@alflennik

Looking good so far.

I have added a couple of commits that update content of the coverage-report.template file so the content is consistent with the editorial requirements of the APG.

Will you also please change the coverage-report.js to put the output into content/about/coverage-and-quality. Also change the name of the generated html file from index.html to coverage-and-quality-report.html. Then, delete the /coverage directory and its content.

After looking more deeply into the report output, I think we might want to consider holding off on merging the coverage-report script and report changes until after we complete both issues #2418 and #2424. That is because I now see there are a lot of links in the coverage reports to the practices. All those links, which are generated by the script, will have to be updated after #2424 is complete. That would mean merging move-examples to main, then changing the base branch for this PR to main, then completing #2424, then rebasing this PR.

If we were to go that route, we would want to break out all the repairs to the example files you made in this branch into a separate branch that we would merge separately into the move-examples branch.

Also, if we were to wait till after both #2418 and #2424 are complete before merging these changes to the coverage and quality reports, then perhaps we would want to include the changes to .github/workflows/coverage-report.yml in this PR instead of in #2464.

@alflennik
Copy link
Contributor Author

@mcking65 I made the changes you requested. About whether to merge the PR now or wait, I think since the coverage report is currently working my preference is to merge this PR now and do a separate PR addressing the repercussions after the aria-practices.html file is fully removed.

Copy link
Contributor

@mcking65 mcking65 left a comment

Choose a reason for hiding this comment

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

@alflennik

There are two issues remaining:

  1. The links in the generated output have incorrect href values; I get file not found when following them.
  2. The output is different from the output in main. Content has not changed, so the report should be the same.

Compare generated out in main: coverage/index.html in main
To generated out output in move-examples-9: content/about/coverage-and-quality/coverage-and-quality-report.html in move-examples-9

It appears that the processing of practices to get information about guidance is not yet updated.

@alflennik
Copy link
Contributor Author

@mcking65 Thanks for the review! You are right about the broken links, and that is now fixed; it occurred when I updated the directory of the coverage report.

About the differences in report content, the reason for that, I think, is that the coverage report GitHub action seems to be disabled and has been disabled for some time (around a year?). Here is a link to the page where the script is marked as disabled.

@mcking65
Copy link
Contributor

@alflennik wrote:

@mcking65 Thanks for the review! You are right about the broken links, and that is now fixed; it occurred when I updated the directory of the coverage report.

Awesome, thank you!!

About the differences in report content, the reason for that, I think, is that the coverage report GitHub action seems to be disabled and has been disabled for some time (around a year?). Here is a link to the page where the script is marked as disabled.

I created branch test-coverage-report from tip of main to test this hypothesis. In that branch, I ran scripts/coverage-report.js against the content in main. That version of the script is listing guidance that is not being listed by the script in move-examples-9.

Compare to /coverage/index.html in test-coverage-report.

Here are two examples of differences that need to be resolved.

Roles with No Guidance or Examples:

  • 29 in the test branch
  • 31 in the move-examples-9 branch, including presentation and tooltip, both of which have guidance in the move-examples-9 content tree that should be listed.

Roles with at Least One Guidance or Example:

  • 13 in the test-coverage branch
  • 17 in the move-examples-9 branch: there are more here because several roles should be listed in the 2 or more table, e.g., alert and cell have guidance in the move-examples-9 branch that is not listed.

@alflennik
Copy link
Contributor Author

The inaccurate coverage numbers were a great catch @mcking65, and thank you for the thorough review. Also thank you for running the coverage report on another branch, having that for reference came in handy. After investigating I found that moving the practices into their own files was not something I had accounted for, because I had opened the PR before that change was made. Now I've updated this branch to load the practices from their own files and I confirmed that the coverage levels are equivalent and that also all the links work on the coverage report.

@mcking65 mcking65 merged commit 5c5f372 into move-examples Oct 27, 2022
@mcking65 mcking65 deleted the move-examples-9 branch October 27, 2022 06:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants