1292299 - Combine and refactor tests for bug 704320

Status: RESOLVED FIXED

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

Description

[Josh Matthews [:jdm]]

dom/base/test/test_bug704320_http_http.html
dom/base/test/test_bug704320_http_https.html
dom/base/test/test_bug704320_https_https.html
dom/base/test/test_bug704320_https_http.html

These four test files are identical, with the exception of the URLs that they load. The URLs themselves are repeated inside each test file, except for the policy that is appended to the query string. We should combine the files into one, and share the common parts of the URLs in variables so that it's easy to tell precisely what each test URL is verifying.

The tests can be run using `./mach mochitest dom/base/test/test_bug704320_http_http.html` (substituting the appropriate filename as necessary).

Comment 1

[Horatiu Lazu]

Hi, I would like to work on this bug. Is this possible?

Comment 2

[Josh Matthews [:jdm]]

Please do! Ask questions about anything that's unclear!

Comment 3

[Horatiu Lazu]

Hi Josh. Thanks for everything, just have a quick question. I'm assuming I make a new file, such as `test_bug704320.html` - if I do mochitest on that new file is it supposed to actually execute the tests? I tried it myself and I couldn't get it to run anything, am I doing something wrong? Thanks a lot.

Comment 4

[Josh Matthews [:jdm]]

You need to add it to the list in mochitest.ini first, unfortunately.

Comment 5

[Horatiu Lazu]

Attachment: bug1292299Refactoring.diff

Hi, I made my adjustments, please let me know if I need to fix anything. Thanks! Sorry for the delay.

Comment 6

[Josh Matthews [:jdm]]

Comment on attachment 8780063 [details] [diff] [review]
bug1292299Refactoring.diff

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

Great work! Thanks for doing this cleanup!

Comment 7

[Josh Matthews [:jdm]]

Do you have access to the TryServer (https://wiki.mozilla.org/ReleaseEngineering/TryServer#Try_Server) to run the new test on our continuous integration build machines? If not, please apply for it and I will vouch for you. Also, in future setting the review flag for a patch is enough - no need to use the needinfo request as well.

Comment 8

[Horatiu Lazu]

Thank you very much, it was fun! I did the request here: https://bugzilla.mozilla.org/show_bug.cgi?id=1294980

Comment 9

[Horatiu Lazu]

I will be gone for a week, but once I come back from vacation (and you as well) I will push the patch to the TryServer. Thanks.

Comment 10

[Horatiu Lazu]

Hi Josh, I pushed the patch to the TryServer, here's the link: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef3bb7e3994c

Did all go well? Anything else I should do? Thanks!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/94bf7c3ec5aa
Refactor the test cases from bug 704320. r=jdm

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/94bf7c3ec5aa

Comment 13

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/mozilla-central/rev/394f02edb7cb - your new test permanently times out on Android, https://treeherder.mozilla.org/logviewer.html#?job_id=35351647&repo=mozilla-inbound or in the simpler quieter form https://treeherder.mozilla.org/logviewer.html#?job_id=35355717&repo=mozilla-inbound

Comment 14

[Horatiu Lazu]

Hi Josh, is there anything I should do about this?

Comment 15

[Horatiu Lazu]

Attachment: bug1292299Refactoring.diff

Hi, I think I fixed the issue, since multiple tests were merged into one file then it makes sense to increase the time limit (as it timed out in Android). I hope this works!

Comment 16

[Josh Matthews [:jdm]]

Sorry for not responding sooner; I was on vacation last week. Have you tried running the test with the longer timeout on the tryserver on android?

Comment 17

[Josh Matthews [:jdm]]

Comment on attachment 8791348 [details] [diff] [review]
bug1292299Refactoring.diff

requestLongerTimeout would need to be called at the start of the test, or it will time out before reaching it :)

Comment 18

[Horatiu Lazu]

Attachment: bug1292299Refactoring.diff

Hi, thanks! I did the adjustment, I also pushed it to the TryServer and it looks like it passed. Here's the link: https://treeherder.mozilla.org/#/jobs?repo=try&revision=89afc1a06b136fb9bf93bee92a134c9ea479e08f

Comment 19

[Josh Matthews [:jdm]]

Comment on attachment 8792495 [details] [diff] [review]
bug1292299Refactoring.diff

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

Thanks! Please attach a version of the patch with a commit message like "Bug 1292299 - Combine tests for bug 704320 into a single file. r=jdm" and we'll land it again.

Comment 20

[Horatiu Lazu]

Attachment: bug1292299Refactoring.diff

Hi Josh, I improved the commit message. Hope this is good!

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a9995559d5e
Combine tests for bug 704320 into a single file. r=jdm

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8a9995559d5e