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

WPT test summary widget double-counts tests named twice #2303

Closed
stephenmcgruer opened this issue Jun 21, 2022 · 2 comments · Fixed by #2304
Closed

WPT test summary widget double-counts tests named twice #2303

stephenmcgruer opened this issue Jun 21, 2022 · 2 comments · Fixed by #2304

Comments

@stephenmcgruer
Copy link
Contributor

stephenmcgruer commented Jun 21, 2022

This could arguably be user (spec writer?) error, but...

I just discovered <wpt>, and it's a really neat feature that I'm excited to integrate into Secure Payment Confirmation's spec. However, I discovered that the test 'summary' view does not seem to deduplicate test names for its summary numbers, and so effectively restricts one from naming the same test twice in a spec.

image

(Example from SPC - we only have 48 tests in total, but when I linked the same test file from multiple places it counted each instance separately to get 71 tests instead! And I have no idea what happened to Firefox or Safari...).

A reasonable answer might be 'split up your test files more', but although we are definitely guilty of too much behavior tested in one test, I think it is reasonable for a test file to test more than a single location in a specification.

Perhaps the test summary component could throw the test file names into a set() before computing the summary?

@tabatkins
Copy link
Collaborator

Being able to list a test twice (to put a cross-cutting test next to both things it exercises, for example) is absolutely a supported use case, so this is a bug on my part.

stephenmcgruer added a commit to stephenmcgruer/bikeshed that referenced this issue Jun 21, 2022
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
Copy link
Contributor Author

Being able to list a test twice (to put a cross-cutting test next to both things it exercises, for example) is absolutely a supported use case, so this is a bug on my part.

Good to hear :). I've sent a PR for this that I hope is correct, so please (when you have time, no rush!) take a look and lmk. I think it does need your permission to run the checks as I am a 'first time contributor' here :D.

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 a pull request may close this issue.

2 participants