Closed Bug 1269812 Opened 4 years ago Closed 4 years ago

enable test_bug383369.html and test_unsecureRedirect.html for e10s

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
e10s + ---
firefox48 --- fixed
firefox49 --- fixed

People

(Reporter: keeler, Assigned: keeler)

References

(Blocks 1 open bug)

Details

(Whiteboard: [psm-assigned])

Attachments

(1 file)

test_bug383369.html and test_unsecureRedirect.html aren't running in e10s-mode right now. AFAICT test_unsecureRedirect.html works fine with no changes, but test_bug383369.html needs some slight modifications.
Blocks: e10s-tests
tracking-e10s: --- → +
Comment on attachment 8748334 [details]
MozReview Request: bug 1269812 - e10s-ify test_bug383369.html and test_unsecureRedirect.html r=Cykesiopka r=mrbkap

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50203/diff/1-2/
Attachment #8748334 - Attachment description: MozReview Request: bug 1269812 - e10s-ify test_bug383369.html and test_unsecureRedirect.html → MozReview Request: bug 1269812 - e10s-ify test_bug383369.html and test_unsecureRedirect.html r?Cykesiopka
Attachment #8748334 - Flags: review?(cykesiopka.bmo)
Comment on attachment 8748334 [details]
MozReview Request: bug 1269812 - e10s-ify test_bug383369.html and test_unsecureRedirect.html r=Cykesiopka r=mrbkap

https://reviewboard.mozilla.org/r/50203/#review47409

Looks good!
(Admittedly I don't know much about e10s-ifying tests, but the tests now work, and the changes look correct to me, so good enough.)

::: security/manager/ssl/tests/mochitest/mixedcontent/test_bug383369.html:10
(Diff revision 2)
>    <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
>    <script type="text/javascript" src="mixedContentTest.js"></script>
>    <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
>  
>    <script class="testbody" type="text/javascript">
> +  /* global sendAsyncMessage */

Nit: sendAsyncMessage() isn't really global, so please add a comment that this is here just to suppress ESLint no-undef checks.
Attachment #8748334 - Flags: review?(cykesiopka.bmo) → review+
Thanks! r? to mrbkap for the e10s-ifying aspect.
Attachment #8748334 - Flags: review?(mrbkap) → review+
Comment on attachment 8748334 [details]
MozReview Request: bug 1269812 - e10s-ify test_bug383369.html and test_unsecureRedirect.html r=Cykesiopka r=mrbkap

https://reviewboard.mozilla.org/r/50203/#review47643

::: security/manager/ssl/tests/mochitest/mixedcontent/test_bug383369.html:70
(Diff revision 2)
> -  function afterNavigationTest()
> +  function afterNavigationTest() {}
> -  {
> -  }
>  
> -  testCleanUp = function cleanup()
> -  {
> +  testCleanUp = function cleanup() {
> +    SpecialPowers.loadChromeScript(function() {

I tend to prefer a single script that initializes itself and has a "cleanup" message that you can call sendSyncMessage for, but that's a matter of style, so I leave that up to you.
https://reviewboard.mozilla.org/r/50203/#review47409

> Nit: sendAsyncMessage() isn't really global, so please add a comment that this is here just to suppress ESLint no-undef checks.

Thanks - added the comment.
https://reviewboard.mozilla.org/r/50203/#review47643

> I tend to prefer a single script that initializes itself and has a "cleanup" message that you can call sendSyncMessage for, but that's a matter of style, so I leave that up to you.

Thanks for the review. I left the test cleanup as it was so there's less to uplift to aurora, but it does look like some refactoring can be done (this test is the only one that actually defines testCleanUp, so it doesn't seem necessary as generic functionality any more).
Comment on attachment 8748334 [details]
MozReview Request: bug 1269812 - e10s-ify test_bug383369.html and test_unsecureRedirect.html r=Cykesiopka r=mrbkap

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50203/diff/2-3/
Comment on attachment 8748334 [details]
MozReview Request: bug 1269812 - e10s-ify test_bug383369.html and test_unsecureRedirect.html r=Cykesiopka r=mrbkap

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50203/diff/3-4/
Attachment #8748334 - Attachment description: MozReview Request: bug 1269812 - e10s-ify test_bug383369.html and test_unsecureRedirect.html r?Cykesiopka → MozReview Request: bug 1269812 - e10s-ify test_bug383369.html and test_unsecureRedirect.html r=Cykesiopka r=mrbkap
https://hg.mozilla.org/mozilla-central/rev/c6251c984ad6
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Comment on attachment 8748334 [details]
MozReview Request: bug 1269812 - e10s-ify test_bug383369.html and test_unsecureRedirect.html r=Cykesiopka r=mrbkap

Approval Request Comment
[Feature/regressing bug #]: e10s
[User impact if declined]: less e10s test coverage
[Describe test coverage new/current, TreeHerder]: this is the test
[Risks and why]: low - it's just a test
[String/UUID change made/needed]: none
Attachment #8748334 - Flags: approval-mozilla-aurora?
Comment on attachment 8748334 [details]
MozReview Request: bug 1269812 - e10s-ify test_bug383369.html and test_unsecureRedirect.html r=Cykesiopka r=mrbkap

Oh, wait, I guess since these changes are test-only, I don't need to wait for approval for this.
Attachment #8748334 - Flags: approval-mozilla-aurora?
You need to log in before you can comment on or make changes to this bug.