Closed Bug 1161221 Opened 8 years ago Closed 8 years ago

Split meta referrer tests


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

Not set



Tracking Status
firefox41 --- fixed


(Reporter: ckerschb, Assigned: franziskus)




(1 file, 2 obsolete files)

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

Assignee: nobody → franziskuskiefer
Blocks: 704320
Depends on: 1163743
Attached patch splitting http and https tests (obsolete) — Splinter Review
splitted test_bug704320.html in http and https tests and enabled http tests on b2g and both for e10s
Flags: needinfo?(mozilla)
See Also: → 1100609
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 ( not working 1162353) android(times out, bug 1100609) e10s(randomly fails, bug 1100362)

same here, please remove the 'e10s' comment
Attachment #8604390 - Flags: feedback+
Clearing the flag.
Flags: needinfo?(mozilla)
See Also: → 1100362
Attached patch splitting referrer policy tests (obsolete) — Splinter Review
note that this requires patches from bug 1163743 being applied!
Attachment #8604390 - Attachment is obsolete: true
Attachment #8606546 - Flags: review?(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:

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 ( 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.
Attachment #8606546 - Flags: review?(sstamm) → review+
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:
Attachment #8606546 - Attachment is obsolete: true
Attachment #8609489 - Flags: review?(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?
oh, missed that :) no, should be all good
Keywords: checkin-needed
Comment on attachment 8609489 [details] [diff] [review]

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

carries over
Attachment #8609489 - Flags: review?(sstamm) → review+
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.