-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Remove support for stub-* files in the manifest #18746
Conversation
Thanks for the review, @jgraham. When verifying that an incremental update would work I found that it didn't and pushed some changes to deal with that. Is that fine, or should we bump the manifest version? |
Oh we don't handle a whole test type being dropped. Hmm, seems a shame to leave that code in until we next bump the manifest version, but maybe not too bad. Add a comment indicating it should be removed when the manifest version is higher than whatever the current version is. @gsnedders may disagree of course. |
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.
It seems deliberate that we don't handle test types being removed, though we are pretty inconsistent with validating the JSON conforms to the expected schema.
Can you add a TODO(MANIFESTv7) to the comment so we remember to drop it when we bump the version?
The manifest support was added in commit 2650e63, ensuring they aren't treated as tests. Part 2 of web-platform-tests/rfcs#27.
893679c
to
04fbed9
Compare
@gsnedders do you want to review again? |
@gsnedders merging now, please comment after the fact if you don't like what I did. |
The manifest support was added in commit 2650e63,
ensuring they aren't treated as tests.
Part 2 of web-platform-tests/rfcs#27.