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

RESOLVED FIXED in mozilla30

Status

()

enhancement
RESOLVED FIXED
6 years ago
4 months ago

People

(Reporter: geekboy, Assigned: yeukhon)

Tracking

(Blocks 1 bug)

Trunk
mozilla30
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

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
Blocks: CSP
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.
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...)
Assignee: nobody → yeukhon
Status: NEW → ASSIGNED
Attachment #8368955 - Flags: feedback?(sstamm)
Brief try build (in-progress): https://tbpl.mozilla.org/?tree=Try&rev=fc20f9457222

Local builds look good (tested for each patch)
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!
did you use hg rename/move/mv or did you just delete and addremove the files?
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.
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 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.
Attachment #8368955 - Flags: feedback?(sstamm)
Posted patch move_csp_test_v2.patch (obsolete) — Splinter Review
As discussed, I am using Mercurial Distributed SCM (version 2.8.2).

Try looks good: https://tbpl.mozilla.org/?tree=Try&rev=4412ef6c0137
Attachment #8368955 - Attachment is obsolete: true
Attachment #8368956 - Attachment is obsolete: true
Attachment #8368957 - Attachment is obsolete: true
Attachment #8368958 - Attachment is obsolete: true
Attachment #8368960 - Attachment is obsolete: true
Attachment #8370587 - Flags: review?(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.
Attachment #8370587 - Flags: review?(sstamm) → review+
Posted patch move_csp_test_v3.patch (obsolete) — Splinter Review
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
Attachment #8370587 - Attachment is obsolete: true
Attachment #8375192 - Flags: review+
Keywords: checkin-needed
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
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
Attachment #8377019 - Flags: review+
Attachment #8375192 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/01df9ef2b404
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.