Closed Bug 1496636 Opened 6 years ago Closed 6 years ago

add_task() in script in <head><script src></head> will prevent add_task() in <body><script></script></body> to register

Categories

(Testing :: Mochitest, defect)

Product:
Testing
Component:
Mochitest
Version:
Version 3
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox64 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla64
Tracking Flags:
Tracking Status
firefox64 --- fixed

People

(Reporter: timdream, Assigned: timdream)

References

Details

Attachments

(2 files)

patch
2.66 KB, patch
Details | Diff | Splinter Review
Bug 1496636 - add_task() should wait for document finishes loading before starting the test r=florian
46 bytes, text/x-phabricator-request
Details | Review
Attached patch patch — — Splinter Review
STR: 1. apply my patch 2. ./mach test testing/mochitest/tests/Harness_sanity/test_add_task_in_head.html Expected: 1. Test pass with one assertion Actual: [SimpleTest.finish()] No checks actually run. (You need to call ok(), is(), or similar functions at least once. Make sure you use SimpleTest.waitForExplicitFinish() if you need it.) Note: * You can see the <script> block does get executed given that the info() prints out. * SimpleTest.executeSoon() can work around this. I am not sure this is a bug in AddTask.js or HTML parsing/script execution, because I can see "Leaving test noop" shows up before info(), yet these async should not block inline JS execution.
* these async function should not
Perhaps they didn't "block" the next <script> block, but rather because of each <script> block is a macrotask of its own. The following HTML produces console.log in the correct order (1-2-3) since Promise is scheduling an microtask. data:text/html,<script>console.log(1); Promise.resolve().then(() => console.log(2))</script><script>console.log(3)</script> and the following HTML produces correct order too (1-2-3) because we are using setTimeout() to schedule another macrotask. data:text/html,<script>console.log(1); setTimeout(() => console.log(3))</script><script>console.log(2)</script> I checked AddTask.js and it does seem like what AddTask.js is doing. Do our setTimeout() in mochitest behaves differently than in the wild?
The workaround does not really fix the problem :'(. I suspect the place where setTimeout() schedule the macrotask could actually depend on when the parser adds the inline <script> block to the task queue.
Yeah, the fix in AddTask.js worked.
Assignee: nobody → timdream
Status: NEW → ASSIGNED
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #5) > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=673a21f58a1fdd8df76e4f363fe507571836b298 A few tests need adjustment because they are waiting for onload or DOMContentLoaded event in the test function. With the patch those can be removed.
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #8) > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=0068860080c161c4aefdcc24a93155dc934b50a3 Looks like there are still a few failures.
Pushed by tchien@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/48f36c285594 add_task() should wait for document finishes loading before starting the test r=florian
(In reply to Florian Quèze [:florian] from comment #9) > Looks like there are still a few failures. I fixed layout/style/test/chrome/test_display_mode_reflow.html before landing. Other failure looks unrelated.
https://hg.mozilla.org/mozilla-central/rev/48f36c285594
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox64: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: