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

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

Merged
merged 3 commits into from Mar 24, 2020

Conversation

@stephenmcgruer
Copy link
Contributor

@stephenmcgruer stephenmcgruer commented Mar 16, 2020

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 temporarily deployed to wpt-preview-22284 Mar 16, 2020 Inactive
@stephenmcgruer stephenmcgruer force-pushed the smcgruer/fix_testharnessreport branch from cf8dac3 to 439d101 Mar 23, 2020
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-22284 Mar 23, 2020 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

@stephenmcgruer stephenmcgruer commented Mar 23, 2020

@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

@stephenmcgruer stephenmcgruer commented Mar 23, 2020

@stephenmcgruer
Copy link
Contributor Author

@stephenmcgruer stephenmcgruer commented Mar 23, 2020

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 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 Mar 23, 2020 Inactive
@stephenmcgruer stephenmcgruer force-pushed the smcgruer/fix_testharnessreport branch from 21b1f16 to 3441a44 Mar 23, 2020
@wpt-pr-bot
Copy link
Collaborator

@wpt-pr-bot wpt-pr-bot commented Mar 23, 2020

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 Mar 23, 2020 Inactive
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-22284 Mar 23, 2020 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
@@ -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>

This comment has been minimized.

@Hexcles

Hexcles Mar 23, 2020
Member

This LGTM.

@stephenmcgruer
Copy link
Contributor Author

@stephenmcgruer stephenmcgruer commented Mar 23, 2020

@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 domenic commented Mar 23, 2020

Awesome, those look exactly right!

This reverts commit 2028a71.
@stephenmcgruer stephenmcgruer merged commit 38df7bb into master Mar 24, 2020
11 checks passed
11 checks passed
detect-deployment
Details
Azure Pipelines Build #20200324.3 succeeded
Details
Azure Pipelines (./wpt test-jobs) ./wpt test-jobs succeeded
Details
Azure Pipelines (affected tests without changes: Safari Technology Preview) affected tests without changes: Safari Technology Preview succeeded
Details
Azure Pipelines (affected tests: Safari Technology Preview) affected tests: Safari Technology Preview succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests) wpt.fyi hook: safari-preview-affected-tests succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests-without-changes) wpt.fyi hook: safari-preview-affected-tests-without-changes succeeded
Details
Community-TC (pull_request) TaskGroup: success
Details
wpt.fyi - chrome[experimental] Chrome results
Details
wpt.fyi - firefox[experimental] Firefox results
Details
wpt.fyi - safari[experimental] Safari results
Details
@stephenmcgruer stephenmcgruer deleted the smcgruer/fix_testharnessreport branch Mar 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants