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

Jump to bottom

Remove testharnessreport.js include in origin-policy tests subframe #22284

Merged
merged 3 commits into from Mar 24, 2020
Merged

Remove testharnessreport.js include in origin-policy tests subframe #22284

merged 3 commits into from Mar 24, 2020

Conversation

stephenmcgruer
Copy link
Contributor

@stephenmcgruer stephenmcgruer commented Mar 16, 2020 •
edited

Including the testharnessreport.js here prevents the subframe tests from being reported to the main frame, which means wpt run misses them. Removing it shows a bunch of previously unreported tests.

Fixes #22113

@stephenmcgruer stephenmcgruer self-assigned this Mar 16, 2020
@wpt-pr-bot wpt-pr-bot added infra origin-policy wptrunner The automated test runner, commonly called through ./wpt run labels Mar 16, 2020
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-22284 March 16, 2020 21:09 Inactive
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-22284 March 23, 2020 17:48 Inactive
@stephenmcgruer stephenmcgruer changed the title [WIP] Do not filter all events in wptrunner's testharnessreport.js Do not filter all events in wptrunner's testharnessreport.js Mar 23, 2020
@stephenmcgruer
Copy link
Contributor Author

@jgraham PTAL. I couldn't figure out a better way to do this (or to test it); open to ideas.

If message_events are going to be necessary for correct code, we may want to just remove the ability to set them via setup. I found one location using message_events; intersection-observer/resources/observer-in-iframe-subframe.html.

@stephenmcgruer
Copy link
Contributor Author

I fired off a trigger run on chrome dev; https://community-tc.services.mozilla.com/tasks/groups/JIQr4Y0pTmSPJnTQ7ckG2Q

@stephenmcgruer
Copy link
Contributor Author

https://wpt.fyi/results/?diff&filter=ADC&run_id=448580001&run_id=438890001 should be a comparison of the change to the closest SHA without it.

@Hexcles
Copy link
Member

Hexcles commented Mar 23, 2020

Ahh right we don't actually need testharnessreport.js in iframes. This looks promising.

@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-22284 March 23, 2020 20:13 Inactive
@wpt-pr-bot
Copy link
Collaborator

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 wpt-pr-bot temporarily deployed to wpt-preview-22284 March 23, 2020 20:55 Inactive
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-22284 March 23, 2020 21:12 Inactive
@stephenmcgruer stephenmcgruer changed the title Do not filter all events in wptrunner's testharnessreport.js Remove testharnessreport.js include in origin-policy tests subframe Mar 23, 2020
Hexcles
Hexcles approved these changes Mar 23, 2020
View reviewed changes
origin-policy/resources/subframe-with-origin-policy.py
@@ -20,7 +20,6 @@ def main(request, response):
<title>Origin policy subframe</title>

<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
Copy link
Member

Choose a reason for hiding this comment

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

This LGTM.

@stephenmcgruer
Copy link
Contributor Author

@domenic - can you look at the checks for this PR (https://github.com/web-platform-tests/wpt/pull/22284/checks) and see if the diffs are what you would expect?

@domenic
Copy link
Member

domenic commented Mar 23, 2020

Awesome, those look exactly right!

@stephenmcgruer stephenmcgruer merged commit 38df7bb into master Mar 24, 2020
@stephenmcgruer stephenmcgruer deleted the smcgruer/fix_testharnessreport branch March 24, 2020 00:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hexcles Hexcles Hexcles approved these changes

@jgraham jgraham Awaiting requested review from jgraham

@KyleJu KyleJu Awaiting requested review from KyleJu

@LukeZielinski LukeZielinski Awaiting requested review from LukeZielinski

Assignees

@jgraham jgraham

Labels
infra origin-policy status:needs-reviewers wptrunner The automated test runner, commonly called through ./wpt run
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Origin policy tests not running on wpt.fyi
5 participants
@stephenmcgruer @Hexcles @wpt-pr-bot @domenic @jgraham