Upstream Blink's 'securitypolicyviolation' tests.

Upstream Blink's 'securitypolicyviolation' tests.

Starting on the long slog to get our CSP layout tests more completely
upstreamed. This patch leaves most upstream CSP tests disabled, as I
still need to go through them in detail, but carves out the
'securitypolicyviolation' directory, moves our local tests over, and
ports them to `testharness.js`-style.

BUG=695486
Review-Url: https://codereview.chromium.org/2711913003
Cr-Commit-Position: refs/heads/master@{#452780}
Committed: https://chromium.googlesource.com/chromium/src/+/7e900589febbfc49da56f18b33750786c24ba14c

[Mike West, 2017-02-23 15:44:14]

The CQ bit was checked by mkwst@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 15:44:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mike West, 2017-02-23 15:45:56]

mkwst@chromium.org changed reviewers:
+ foolip@chromium.org, rbyers@chromium.org

[Mike West, 2017-02-23 15:45:56]

Hey Rick, Philip! Mind taking a look at this hopefully-straightforward port of a
few CSP tests to WPT?

I'm most interested in your comments on the general strategy of carving out one
directory at a time and deduping. An alternative would be to move all the
non-deduped tests into some other directory and skip it, but that seems like it
would bring a bit too much of Blink's workflow into WPT...

WDYT?

[Mike West, 2017-02-23 15:46:22]

Also, every time I add tests, MANIFEST.json grows by millions of lines. That
seems odd. Am I holding it wrong?

[foolip, 2017-02-23 16:45:50]

On 2017/02/23 15:46:22, Mike West (sloooooow) wrote:
> Also, every time I add tests, MANIFEST.json grows by millions of lines. That
> seems odd. Am I holding it wrong?

Yes, but it's barely possibly to hold it right, see
https://groups.google.com/a/chromium.org/d/msg/blink-dev/dsupejcnSHw/wsDBL5NF...
for some details about what's wrong and how to cope. Basically, just commit
whatever is generated, possibly except the hilarious MANIFEST.json entry itself,
and repeat as many times as there are conflicts with other CLs.

Fixing this is top priority :)

[foolip, 2017-02-23 16:51:58]

On 2017/02/23 15:45:56, Mike West (sloooooow) wrote:
> Hey Rick, Philip! Mind taking a look at this hopefully-straightforward port of
a
> few CSP tests to WPT?
> 
> I'm most interested in your comments on the general strategy of carving out
one
> directory at a time and deduping. An alternative would be to move all the
> non-deduped tests into some other directory and skip it, but that seems like
it
> would bring a bit too much of Blink's workflow into WPT...
> 
> WDYT?

I think the generic answer is that whatever workflow maximizes the number of
useful tests that end up in web-platform-tests ought to be favored by everyone
involved, so it matters how much hassle it is for you. I think it's OK if the
commit history of web-platform-tests shows some traces of how you went about
this, although renaming tests is nice to avoid if that's part of any possible
plan.

[foolip, 2017-02-23 17:00:34]

Didn't review in depth since this is a rewrite, but lgtm % nits.

https://codereview.chromium.org/2711913003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/securitypolicyviolation/support/inside-worker.sub.js
(right):

https://codereview.chromium.org/2711913003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/securitypolicyviolation/support/inside-worker.sub.js:1:
importScripts("{{location[scheme]}}://{{host}}:{{location[port]}}/resources/testharness.js");
I assume that the string formatting is a feature of wptserve, if it's something
else I'm curious.

https://codereview.chromium.org/2711913003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/securitypolicyviolation/support/inside-worker.sub.js.headers
(right):

https://codereview.chromium.org/2711913003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/securitypolicyviolation/support/inside-worker.sub.js.headers:1:
Expires: Mon, 26 Jul 1997 05:00:00 GMT
A few of these headers weren't in the PHP equivalent, or were they added by
something else?

https://codereview.chromium.org/2711913003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/securitypolicyviolation/targeting.html
(right):

https://codereview.chromium.org/2711913003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/securitypolicyviolation/targeting.html:57:

cat on keyboard?

[commit-bot: I haz the power, 2017-02-23 17:02:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-23 17:02:51]

Dry run: This issue passed the CQ dry run.

[Mike West, 2017-02-23 18:21:51]

On 2017/02/23 at 16:51:58, foolip wrote:
> I think the generic answer is that whatever workflow maximizes the number of
useful tests that end up in web-platform-tests ought to be favored by everyone
involved, so it matters how much hassle it is for you. I think it's OK if the
commit history of web-platform-tests shows some traces of how you went about
this, although renaming tests is nice to avoid if that's part of any possible
plan.

Thanks! I'll give Rick a chance to take a look.

If y'all are happy with this patch, I'd prefer to do it this way. Moving stuff
around in WPT because Blink isn't ready yet seems like a worse way to do things,
as it impacts other vendors for no good reason. :)

https://codereview.chromium.org/2711913003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/securitypolicyviolation/support/inside-worker.sub.js
(right):

https://codereview.chromium.org/2711913003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/securitypolicyviolation/support/inside-worker.sub.js:1:
importScripts("{{location[scheme]}}://{{host}}:{{location[port]}}/resources/testharness.js");
If you mean the `{{...}}` bit, then yes. Files with a `.sub` suffix will do
string substitution on these things, which makes it possible to deal with the
nonstandard ports (and differences between test runner engines).

https://codereview.chromium.org/2711913003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/securitypolicyviolation/support/inside-worker.sub.js.headers
(right):

https://codereview.chromium.org/2711913003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/securitypolicyviolation/support/inside-worker.sub.js.headers:1:
Expires: Mon, 26 Jul 1997 05:00:00 GMT
On 2017/02/23 at 17:00:34, foolip wrote:
> A few of these headers weren't in the PHP equivalent, or were they added by
something else?

They weren't. I guess we don't need them, but I copy/pasted them from somewhere.

https://codereview.chromium.org/2711913003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/securitypolicyviolation/targeting.html
(right):

https://codereview.chromium.org/2711913003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/securitypolicyviolation/targeting.html:57:

On 2017/02/23 at 17:00:34, foolip wrote:
> cat on keyboard?

Nope. This is load-bearing whitespace: the test relies on line numbers matching
up. Yes, this is dumb. :(

[foolip, 2017-02-23 19:43:28]

https://codereview.chromium.org/2711913003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/securitypolicyviolation/targeting.html
(right):

https://codereview.chromium.org/2711913003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/securitypolicyviolation/targeting.html:57:

On 2017/02/23 18:21:51, Mike West (sloooooow) wrote:
> On 2017/02/23 at 17:00:34, foolip wrote:
> > cat on keyboard?
> 
> Nope. This is load-bearing whitespace: the test relies on line numbers
matching
> up. Yes, this is dumb. :(

I think it's cool :)

[Rick Byers, 2017-02-23 20:00:16]

On 2017/02/23 18:21:51, Mike West (sloooooow) wrote:
> On 2017/02/23 at 16:51:58, foolip wrote:
> > I think the generic answer is that whatever workflow maximizes the number of
> useful tests that end up in web-platform-tests ought to be favored by everyone
> involved, so it matters how much hassle it is for you. I think it's OK if the
> commit history of web-platform-tests shows some traces of how you went about
> this, although renaming tests is nice to avoid if that's part of any possible
> plan.
> 
> Thanks! I'll give Rick a chance to take a look.

Yeah the overall plan seems reasonable to me.

You're obviously the expert on what the spec says and how best to test it. 
Perhaps it's worth doing a sanity check of the results you get on one other
browser?

> 
> If y'all are happy with this patch, I'd prefer to do it this way. Moving stuff
> around in WPT because Blink isn't ready yet seems like a worse way to do
things,
> as it impacts other vendors for no good reason. :)
> 
>
https://codereview.chromium.org/2711913003/diff/1/third_party/WebKit/LayoutTe...
> File
>
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/securitypolicyviolation/support/inside-worker.sub.js
> (right):
> 
>
https://codereview.chromium.org/2711913003/diff/1/third_party/WebKit/LayoutTe...
>
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/securitypolicyviolation/support/inside-worker.sub.js:1:
>
importScripts("{{location[scheme]}}://{{host}}:{{location[port]}}/resources/testharness.js");
> If you mean the `{{...}}` bit, then yes. Files with a `.sub` suffix will do
> string substitution on these things, which makes it possible to deal with the
> nonstandard ports (and differences between test runner engines).
> 
>
https://codereview.chromium.org/2711913003/diff/1/third_party/WebKit/LayoutTe...
> File
>
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/securitypolicyviolation/support/inside-worker.sub.js.headers
> (right):
> 
>
https://codereview.chromium.org/2711913003/diff/1/third_party/WebKit/LayoutTe...
>
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/securitypolicyviolation/support/inside-worker.sub.js.headers:1:
> Expires: Mon, 26 Jul 1997 05:00:00 GMT
> On 2017/02/23 at 17:00:34, foolip wrote:
> > A few of these headers weren't in the PHP equivalent, or were they added by
> something else?
> 
> They weren't. I guess we don't need them, but I copy/pasted them from
somewhere.
> 
>
https://codereview.chromium.org/2711913003/diff/1/third_party/WebKit/LayoutTe...
> File
>
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/securitypolicyviolation/targeting.html
> (right):
> 
>
https://codereview.chromium.org/2711913003/diff/1/third_party/WebKit/LayoutTe...
>
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/securitypolicyviolation/targeting.html:57:
> 
> On 2017/02/23 at 17:00:34, foolip wrote:
> > cat on keyboard?
> 
> Nope. This is load-bearing whitespace: the test relies on line numbers
matching
> up. Yes, this is dumb. :(

[Mike West, 2017-02-24 07:42:22]

On 2017/02/23 at 20:00:16, rbyers wrote:
> On 2017/02/23 18:21:51, Mike West (sloooooow) wrote:
> > On 2017/02/23 at 16:51:58, foolip wrote:
> > > I think the generic answer is that whatever workflow maximizes the number
of
> > useful tests that end up in web-platform-tests ought to be favored by
everyone
> > involved, so it matters how much hassle it is for you. I think it's OK if
the
> > commit history of web-platform-tests shows some traces of how you went about
> > this, although renaming tests is nice to avoid if that's part of any
possible
> > plan.
> > 
> > Thanks! I'll give Rick a chance to take a look.
> 
> Yeah the overall plan seems reasonable to me.
> 
> You're obviously the expert on what the spec says and how best to test it. 
Perhaps it's worth doing a sanity check of the results you get on one other
browser?

Firefox and Edge don't yet implement `securitypolicyviolation`. Safari does, but
gets the targeting wrong. I'm pretty sure the tests are accurate; I'll file
bugs. :)

[Mike West, 2017-02-24 07:42:27]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2017-02-24 07:42:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-24 07:47:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-24 07:47:34]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Mike West, 2017-02-24 08:07:16]

The CQ bit was checked by mkwst@chromium.org

[Mike West, 2017-02-24 08:07:20]

The patchset sent to the CQ was uploaded after l-g-t-m from foolip@chromium.org
Link to the patchset: https://codereview.chromium.org/2711913003/#ps20001
(title: "MANIFEST")

[commit-bot: I haz the power, 2017-02-24 08:07:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-24 09:31:24]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1487923636602960,
"parent_rev": "6fee69fffaad7d1254c67d1a8e1b3daa56a0f3d3", "commit_rev":
"7e900589febbfc49da56f18b33750786c24ba14c"}

[commit-bot: I haz the power, 2017-02-24 09:32:32]

Description was changed from

==========
Upstream Blink's 'securitypolicyviolation' tests.

Starting on the long slog to get our CSP layout tests more completely
upstreamed. This patch leaves most upstream CSP tests disabled, as I
still need to go through them in detail, but carves out the
'securitypolicyviolation' directory, moves our local tests over, and
ports them to `testharness.js`-style.

BUG=695486
==========

to

==========
Upstream Blink's 'securitypolicyviolation' tests.

Starting on the long slog to get our CSP layout tests more completely
upstreamed. This patch leaves most upstream CSP tests disabled, as I
still need to go through them in detail, but carves out the
'securitypolicyviolation' directory, moves our local tests over, and
ports them to `testharness.js`-style.

BUG=695486

Review-Url: https://codereview.chromium.org/2711913003
Cr-Commit-Position: refs/heads/master@{#452780}
Committed:
https://chromium.googlesource.com/chromium/src/+/7e900589febbfc49da56f18b3375...
==========

[commit-bot: I haz the power, 2017-02-24 09:32:33]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/7e900589febbfc49da56f18b3375...