-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
[fetch] Introduce test generation tool & add more tests for metadata request headers #25247
[fetch] Introduce test generation tool & add more tests for metadata request headers #25247
Conversation
Also CC @lweichselbaum (sorry Lukas: GitHub didn't allow me to designate you as a reviewer) |
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 reviewed earlier iterations on this PR in detail at bocoup#13, and I'm quite supportive of landing it. Thank you for the hard work you've done to get this into reasonable shape, @jugglinmike! LGTM.
The check named, "wpt-decision-task" failed on the latest commit, reporting:
It had a similar problem with the prior head of this branch, though it was successful when I initially opened this pull request. Both of the failed commands work locally, so this may be related to branch availability. This branch is located on a fork of WPT. @Hexcles (hello!) are you familiar with the failing task? |
I saw a similar failure on my PR just now but forgot to keep the link. Closing and reopening the PR seems to have fixed it. |
Thanks; I'll give that a try |
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.
Thanks for doing this, Mike!
I really like the readability of the config format in fetch/metadata/tools/fetch-metadata.conf.yml.
I'm glad to hear that, @arturjanc. The configuration file is where the complexity of the generation logic is most apparent, so I really tried to make it as clear as possible. There's still a lot to take in, but it's encouraging to hear that someone other than me can make some sense of it. |
From what I understand, this is reviewed and ready to land. The stability checks failed I believe because this introduces too many tests and so the stability checks time out. @jugglinmike is currently working on a different project. Worst case if these tests somehow catch fire, we can revert. Any objection to merging? |
Ha! I thought this landed months ago. Yes. Please merge it. :) |
5a8ad43
to
e4927b8
Compare
@jugglinmike I've rebased this branch to resolve conflicts, can you enable "Allow edits and access to secrets by maintainers" so I can push those? (@zcorpan asked me) |
bd4858e
to
7f3c22d
Compare
https://wpt.fyi/results/fetch/metadata?label=pr_head&max-count=1&pr=25247 show the results. Everything looks OK except https://wpt.fyi/results/fetch/metadata/generated/appcache-manifest.https.sub.html?label=pr_head&max-count=1&pr=25247 where there's a harness error:
This should be fixed by just not returning a promise, it's already chained correctly to pass or fail the async test. Stability jobs are failing on /webaudio/the-audio-api/the-mediaelementaudiosourcenode-interface/no-cors.https.html because fetch/api/resources/redirect.py was updated, but the test does a regular GET request and I presume was already flaky. I'll admin merge to ignore this failure. |
https://wpt.fyi/results/fetch/metadata/generated/appcache-manifest.https.sub.html?label=pr_head&max-count=1&pr=25247 looks better now, no longer a harness error. |
Thank you @foolip ! |
This patch is the latest in a research project we've been running since the beginning of the year. The
README.md
file included in this patch summarizes the practical usage aspects and the high-level design goals.If you're interested in learning more about the design process, you can check out:
This patch replaces tests that were manually written with tests that have been procedurally generated by the new tool. I've verified that there are no regressions in coverage. To promote transparency on that detail, I've organized each replacement as a distinct commit in this pull request. If/when this is accepted for WPT, I recommend using a merge commit in order to preserve this information.
There are still more tests to be written (both to replace existing tests and to further improve coverage). I will be pushing additional commits to this feature branch over the next few days, though I do not expect to reach completeness. I'll be careful not to rewrite history to avoid disrupting the review process.