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

Only count each WPT test once for the summary view #2304

Merged
merged 4 commits into from
Jun 24, 2022

Conversation

stephenmcgruer
Copy link
Contributor

Previously, every .wpt-name was counted for calculating the pass/total summary,
even if it had previously been seen. This would cause inaccurate results for
specifications where authors list the same WPT test more than once (e.g., if
the test relates to multiple parts of the spec).

Fixes #2303

Previously, every .wpt-name was counted for calculating the pass/total summary,
even if it had previously been seen. This would cause inaccurate results for
specifications where authors list the same WPT test more than once (e.g., if
the test relates to multiple parts of the spec).

Fixes speced#2303
@stephenmcgruer stephenmcgruer marked this pull request as ready for review June 21, 2022 18:22
@stephenmcgruer
Copy link
Contributor Author

I think this is correct; tested locally with SPC and it seems to work. Just discovered I need to rebase the tests however, doing that now.

@stephenmcgruer stephenmcgruer marked this pull request as draft June 21, 2022 18:23
@stephenmcgruer stephenmcgruer marked this pull request as ready for review June 21, 2022 18:30
// Only update the summary pass/total count if we haven't seen this
// test before, to support authors listing the same test multiple times
// in a spec.
if (!seenTests.has(nameEl.getAttribute("title"))) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know the indentation in this file is inconsistent between tabs and spaces (ugh, I dunno how that happened) but that doesn't mean that mixing 4-space and 2-space indents is allowed as well. ^_^ Mind switching it all to tab-based?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops on the 2-space vs 4-space, my bad. Will fix that. I don't see the tabs though; I think my PR introduces the only tabs in the file actually (unless my editor is lying to me?).

Changed to all space-indented, 4-space indentation for now. If you want, I can definitely turn it all into tabs, let me know :).

@tabatkins tabatkins merged commit e974d61 into speced:main Jun 24, 2022
@tabatkins tabatkins mentioned this pull request Oct 21, 2022
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.

WPT test summary widget double-counts tests named twice
2 participants