915824 - move more CSP tests from content/base/test into csp subdir

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement)

Description

[Sid Stamm [:geekboy or :sstamm]]

Most of the CSP tests were moved in bug 903080, but a few got missed 'cause they're named stealthily.

test_bug548193.html
file_bug548193.sjs
file_bug558431.html
file_bug650386_content.sjs
test_bug650386_redirect_301.html
test_bug650386_redirect_302.html
test_bug650386_redirect_303.html
test_bug650386_redirect_307.html
file_bug650386_report.sjs
file_bug702439.html
file_bug717511_2.html
file_bug717511.html

Comment 1

[Garrett Robinson [:grobinson]]

We should rename these tests to be descriptive while we're at it - just have to make sure the bug # is in a comment, preferably at the top of the test_*.html file.

Comment 2

[Yeuk Hon Wong [:yeukhon]]

Attachment: part_1_xcsp_report_in_json_bug548193.patch

I will flag one but there are five parts (each move is a single patch), but they depend on the order (which isn't nice but we can change that if we want to...)

Comment 3

[Yeuk Hon Wong [:yeukhon]]

Attachment: part_2_policyuri_async_fetch_bug558431.patch

Comment 4

[Yeuk Hon Wong [:yeukhon]]

Attachment: part_3_xcsp_does_not_follow_redirect.patch

Comment 5

[Yeuk Hon Wong [:yeukhon]]

Attachment: part_4_subframe_run_inline_if_allowed.patch

Comment 6

[Yeuk Hon Wong [:yeukhon]]

Attachment: part_5_multiple_xcsp_allow_injection_bypass.patch

Comment 7

[Yeuk Hon Wong [:yeukhon]]

Brief try build (in-progress): https://tbpl.mozilla.org/?tree=Try&rev=fc20f9457222

Local builds look good (tested for each patch)

Comment 8

[Yeuk Hon Wong [:yeukhon]]

A quick comment: redirect tests have been "re-implemented" in csp/ directory with a quick glance. Therefore I added xcsp prefix to the file names. Injection is assosicated with XCSP at the time so it was named with xcsp prefix. The other two without xcsp.. I am just waiting to see if we should add xcsp or not..

In general, I think *it might be a good idea* to name XCSP-only tests with xcsp so that when we remove XCSP support from Firefox (I assume this is what we will do!) we can easily remove xcsp...

Sorry for the spam. Thanks!

Comment 9

[Sid Stamm [:geekboy or :sstamm]]

did you use hg rename/move/mv or did you just delete and addremove the files?

Comment 10

[Yeuk Hon Wong [:yeukhon]]

I did hg move (addremove got me in trouble, we can talk about that one on IRC). Or I think I did hg move for most of them. Do they not just show up as R and A in the end?

I also have to fix comments and file path after I did hg move.

Comment 11

[Sid Stamm [:geekboy or :sstamm]]

Look at your raw diffs... they're different for the different techniques.  

regarding xcsp: if the test makes ZERO sense for the spec compliant header, that's fine, use "xcsp" in the file name.  If we can convert it to work with CSP 1.0 or 1.1, then don't because we'll end up tweaking the tests to use the new parser/syntax/etc.

Comment 12

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8368955 [details] [diff] [review]
part_1_xcsp_report_in_json_bug548193.patch

Review of attachment 8368955 [details] [diff] [review]:
-----------------------------------------------------------------

As we discussed in IRC last Friday, please collapse the moves into one patch and use hg mv to relocate the files.  (Please also be sure you've got a recent version of mercurial as briansmith suggested older hg versions can mess up history of mv'ed files.

Comment 13

[Yeuk Hon Wong [:yeukhon]]

Attachment: move_csp_test_v2.patch

As discussed, I am using Mercurial Distributed SCM (version 2.8.2).

Try looks good: https://tbpl.mozilla.org/?tree=Try&rev=4412ef6c0137

Comment 14

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8370587 [details] [diff] [review]
move_csp_test_v2.patch

Review of attachment 8370587 [details] [diff] [review]:
-----------------------------------------------------------------

r=me.  Please make sure the bug #s are preserved in the test files you renamed to not have bug numbers.

::: content/base/test/csp/test_301_redirect.html
@@ +70,5 @@
>  
>  SimpleTest.waitForExplicitFinish();
>  
>  // save this for last so that our listeners are registered.
> +document.getElementById('content_iframe').src = 'file_redirect_content.sjs?301';

Please maintain the bug number in this file somewhere (in a comment at the top would probably be fine).  Actually, please make sure the bug number is referenced somewhere in each file where you change the name from a bug number to its functionality (this way it's easier to search by bug number).

::: content/base/test/csp/test_csp_report.html
@@ +1,5 @@
>  <!DOCTYPE HTML>
>  <html>
> +<!--
> +https://bugzilla.mozilla.org/show_bug.cgi?id=548193
> +-->

Yeah, like this.  This is good.

Comment 15

[Yeuk Hon Wong [:yeukhon]]

Attachment: move_csp_test_v3.patch

Carrying over r+ from Sid.

Added bug numbers to all sjs and html files except ^headers^.

Try looks good: https://tbpl.mozilla.org/?tree=Try&rev=3ef73fce48a5

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0a3fa6d853a6

Comment 17

[Ryan VanderMeulen [:RyanVM]]

This effectively re-enabled a lot of tests that were disabled on B2G because their platform-specific manifests weren't updated. Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e5a57e18585

https://tbpl.mozilla.org/php/getParsedLog.php?id=34626241&tree=Mozilla-Inbound

Comment 18

[Yeuk Hon Wong [:yeukhon]]

Attachment: move_csp_test_v4.patch

Modified b2g.json, b2g-desktop.json and b2g-debug.json to reflect the name changes.

Try looks good, all green: https://tbpl.mozilla.org/?tree=Try&rev=c0d449acd813

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/01df9ef2b404

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/01df9ef2b404