Closed Bug 1292299 Opened 3 years ago Closed 3 years ago

Combine and refactor tests for bug 704320

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: jdm, Assigned: horatiulazu, Mentored)

Details

(Whiteboard: [lang=js][good first bug])

Attachments

(1 file, 3 obsolete files)

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).
Priority: -- → P3
Hi, I would like to work on this bug. Is this possible?
Flags: needinfo?(josh)
Please do! Ask questions about anything that's unclear!
Flags: needinfo?(josh)
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.
Flags: needinfo?(josh)
You need to add it to the list in mochitest.ini first, unfortunately.
Flags: needinfo?(josh)
Attached patch bug1292299Refactoring.diff (obsolete) — Splinter Review
Hi, I made my adjustments, please let me know if I need to fix anything. Thanks! Sorry for the delay.
Flags: needinfo?(josh)
Attachment #8780063 - Flags: review?(josh)
Comment on attachment 8780063 [details] [diff] [review]
bug1292299Refactoring.diff

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

Great work! Thanks for doing this cleanup!
Attachment #8780063 - Flags: review?(josh) → review+
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.
Assignee: nobody → horatiulazu
Flags: needinfo?(josh) → needinfo?(horatiulazu)
Thank you very much, it was fun! I did the request here: https://bugzilla.mozilla.org/show_bug.cgi?id=1294980
Flags: needinfo?(horatiulazu)
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.
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!
Flags: needinfo?(josh)
Flags: needinfo?(josh)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/94bf7c3ec5aa
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla51 → ---
Hi Josh, is there anything I should do about this?
Flags: needinfo?(josh)
Attached patch bug1292299Refactoring.diff (obsolete) — Splinter Review
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!
Flags: needinfo?(josh)
Attachment #8791348 - Flags: review?(josh)
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 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 :)
Attachment #8791348 - Flags: review?(josh) → review-
Attached patch bug1292299Refactoring.diff (obsolete) — Splinter Review
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
Attachment #8792495 - Flags: review?(josh)
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.
Attachment #8792495 - Flags: review?(josh) → review+
Hi Josh, I improved the commit message. Hope this is good!
Attachment #8792644 - Flags: review?(josh)
Attachment #8792644 - Flags: review?(josh) → review+
Keywords: checkin-needed
Attachment #8780063 - Attachment is obsolete: true
Attachment #8791348 - Attachment is obsolete: true
Attachment #8792495 - Attachment is obsolete: true
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a9995559d5e
Combine tests for bug 704320 into a single file. r=jdm
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8a9995559d5e
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.