-
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
Count partial mixins as tested deps #13878
Count partial mixins as tested deps #13878
Conversation
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.
LGTM % suggestion, and can you write a test for this?
There are no reviewers for this pull request. Please reach out on W3C's irc server (irc.w3.org, port 6665) on channel #testing (web client) to get help with this. Thank you! |
@wpt-pr-bot seems to be confused :/ |
I think this change now makes sense to me, but only after you explaining to me in length last week. A bit more comment and a test would be great indeed. That said, I haven't reviewed any other changes in idlharness and it could well be the case that this kind of complexity is totally common in idlharness, given the nature of dependency resolution, in which case it might not be necessary to comment too much. |
a176c39
to
92ae52b
Compare
Faaairly sure that my tinkering around with regressions will result in the results for the last commit being available in staging.wpt.fyi shortly.. EDIT: Nope, that would need to be on my fork... lukebjerring#9 |
00ffeac
to
340b297
Compare
https://staging.wpt.fyi/results/?product=chrome%5Bmaster%5D&product=chrome%40340b29725f&diff @foolip This clearly demonstrates the need for running the affected tests on master, or web-platform-tests/wpt.fyi#722 and other diffing improvements. If you enable diff via the api on this staging instance of #722, and then view filter=ACU, it's quicker to see the regression. We can be cheeky with a few different queries though; |
There's a regression in selection EDIT: Fixed when |
Can this land? |
Yeah, I think the removed tests as seen in the diff are expected. Diff URL for c4f9ad6 is: |
Fixes a bug with #13611 where tested partial interfaces weren't being recognized as dependencies for the interfaces that use the mixins.