1161221 - Split meta referrer tests

Status: RESOLVED FIXED

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

Description

[Christoph Kerschbaumer [:ckerschb]]

Lets split meta referrer tests [1] so we can enable them on more platforms. (currently some of them time out).

[1] http://mxr.mozilla.org/mozilla-central/source/dom/base/test/mochitest.ini#612

Comment 1

[Franziskus Kiefer [:franziskus]]

Attachment: splitting http and https tests

splitted test_bug704320.html in http and https tests and enabled http tests on b2g and both for e10s

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8604390 [details] [diff] [review]
splitting http and https tests

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

Looks good, but now we have a lot of duplicate code code, right? It would be better if we can have three files, one for the comment code and two files for testing (one for http, one for https). Otherwise this is a great start. Happy that we can enable those tests for e10s.

::: dom/base/test/mochitest.ini
@@ +610,5 @@
>  [test_bug698381.html]
>  [test_bug698384.html]
>  [test_bug704063.html]
> +[test_bug704320_http.html]
> +skip-if = toolkit == 'android' # android(times out, bug 1100609) e10s(randomly fails, bug 1100362)

please remove the 'e10s' comment

@@ +612,5 @@
>  [test_bug704063.html]
> +[test_bug704320_http.html]
> +skip-if = toolkit == 'android' # android(times out, bug 1100609) e10s(randomly fails, bug 1100362)
> +[test_bug704320_https.html]
> +skip-if = buildapp == 'b2g' || toolkit == 'android' # b2g (https://example.com not working 1162353) android(times out, bug 1100609) e10s(randomly fails, bug 1100362)

same here, please remove the 'e10s' comment

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Clearing the flag.

Comment 4

[Franziskus Kiefer [:franziskus]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bbfd99efe7cd

Comment 5

[Franziskus Kiefer [:franziskus]]

Attachment: splitting referrer policy tests

note that this requires patches from bug 1163743 being applied!

Comment 6

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8606546 [details] [diff] [review]
splitting referrer policy tests

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

This looks great!  Just a few things.  r=me with these changes.

While you're hacking up the files, please tweak the bug704320.sjs file to stop a race:
http://mxr.mozilla.org/mozilla-central/source/dom/base/test/bug704320.sjs#117

The checkForFinish() function in there *may* run more than once in some situations.  Can you please edit that if() check to only fire once (maybe make another variable window._testFinished and set it to true when it fires, and don't fire if it's true).  That will help stamp out a possible cause of the random orange I saw in your try run.

::: dom/base/test/mochitest.ini
@@ +616,5 @@
> +[test_bug704320_http_https.html]
> +support-files = referrerHelper.js
> +[test_bug704320_https_http.html]
> +support-files = referrerHelper.js
> +skip-if = buildapp == 'b2g'  # b2g (https://example.com not working 1162353)

Nit: please prefix 1162353 with "bug " for easier searching.

::: dom/base/test/referrerHelper.js
@@ +77,5 @@
> +
> +/**
> + * Grabs the results via XHR and checks them
> + */
> +function checkResults2() {

What is the difference between this and checkResults1?  Maybe name them better so it is more obvious?  Perhaps checkResultsWithExpected() and checkResultsWithGlobalExpected()?

@@ +102,5 @@
> +	});
> +}
> +
> +
> +var expectedResults = {

I know you probably didn't name this, but since it's a global and it's in a .js file, please make its name all caps so it is easy to differentiate between this variable and any local vars or parameters.

Comment 7

[Franziskus Kiefer [:franziskus]]

Attachment: bug-1161221.patch

think I addressed everythin. Also seems the last patch was missing the remove of test_bug704320.html

let's see if the random oranges are gone:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dfa325ebd62e

Comment 8

[Sid Stamm [:geekboy or :sstamm]]

I gave you an r+ in the previous review, you don't have to re-flag me unless you really want me to do another review.  Did you need me to look at it again?

Comment 9

[Franziskus Kiefer [:franziskus]]

oh, missed that :) no, should be all good

Comment 10

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8609489 [details] [diff] [review]
bug-1161221.patch

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

carries over

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/27ebc65071e5

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/27ebc65071e5