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

Made the trusted-types tests run correctly in a vanilla wptrunner. #14400

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

koto
Copy link
Contributor

@koto koto commented Dec 6, 2018

Tests that set up trusted-types-enforcing CSP now execute in a separate iframe, not to clash with the harness using the DOM directly.

Tests that set up trusted-types-enforcing CSP now execute in a separate iframe, not to clash with the harness using the DOM directly.
@mikewest
Copy link
Member

mikewest commented Dec 7, 2018

@otherdaniel, mind taking a look?

@mikewest
Copy link
Member

mikewest commented Dec 7, 2018

(We talked about this briefly yesterday, and @otherdaniel had interesting ideas about modifying testharness.js instead of all the tests.)

@jgraham
Copy link
Contributor

jgraham commented Dec 7, 2018

OOI what are the requirements here?

@mikewest
Copy link
Member

mikewest commented Dec 7, 2018

@jgraham: Trusted Types gives developers the ability to change the requirements for things like innerHTML. The test harness uses innerHTML to dump out a table of results. This explodes in tests that disable string-based innerHTML rendering. @otherdaniel's approach would be to teach testharness.js to use trusted types if the browser supports them, just as any other library would want to do.

@koto
Copy link
Contributor Author

koto commented Dec 7, 2018 via email

@otherdaniel
Copy link
Member

My version of solving this in testharness ends up looking almost exactly like koto's, with likely the same drawbacks. (https://chromium-review.googlesource.com/c/chromium/src/+/1371874) The main plus would be that I've modified all the tests to allow for a 'testharness' policy, so that all WPT users can be sure that a policy with that name can be built for testharness' own use.

On second thought, I'm no longer sure that's the better path than what koto suggests here.

@koto
Copy link
Contributor Author

koto commented Dec 11, 2018

The patch from the Chromium version would be incomplete for the vanilla WPT - here testharness.js needs to be modified as well (https://github.com/WICG/trusted-types/blob/master/tests/platform-tests/platform-tests.patch has the relevant changes). Alternatively setup({output: false}) called in the test files to disable debug output might work.

Additionally, the vanilla testharnessreport.js breaks with TT as it assigns the data to script element textContent property here.

I have a strong suspicion that this line in chromedriver will also pose problems, I've encountered this in some of the test runs, but I don't understand the whole harness setup fully.

@koto
Copy link
Contributor Author

koto commented Jan 10, 2019

Friendly ping on this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants