Closed
Bug 1316629
Opened 8 years ago
Closed 8 years ago
dom/html/test/test_img_complete.html fails to run >1 time in a browser session
Categories
(Core :: DOM: Core & HTML, defect, P3)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla53
People
(Reporter: jmaher, Assigned: jmaher)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
4.84 KB,
patch
|
jgraham
:
review+
|
Details | Diff | Splinter Review |
in experimenting with a possible new test verification job, I examined existing tests and realized that many do not run >1 time in the same browser session:
./mach mochitest dom/html/test/test_img_complete.html --repeat 1
I get this error at the end of my mach command:
26 INFO TEST-UNEXPECTED-FAIL | dom/html/test/test_img_complete.html | Now that we're loading we should no longer be complete
doing something like this fixes the issue:
diff --git a/dom/html/test/test_img_complete.html b/dom/html/test/test_img_complete.html
--- a/dom/html/test/test_img_complete.html
+++ b/dom/html/test/test_img_complete.html
@@ -19,17 +19,18 @@ https://bugzilla.mozilla.org/show_bug.cg
<div id="content" style="display: none">
</div>
<pre id="test">
<script type="application/javascript">
addLoadEvent(function() {
var img = document.querySelector("img");
ok(img.complete, "By onload, image should have loaded");
- img.src = "image.png?cachebust";
+ var salt = Math.random();
+ img.src = "image.png?cachebust" + salt;
ok(!img.complete, "Now that we're loading we should no longer be complete");
img.onload = function() {
ok(img.complete, "The new thing should have loaded.");
SimpleTest.finish();
}
});
</script>
</pre>
the problem is that seems super hacky- I would rather at the end of the test remove the item from the cache or find a way to ensure we always load a fresh image.png.
:bz, I see you wrote this test originally last year, any thoughts on what you would prefer we do to solve this bug?
Flags: needinfo?(bzbarsky)
![]() |
||
Comment 1•8 years ago
|
||
The right thing here is:
img.src = `image.png?${Math.random()}`; // Make sure we miss the sync image cache.
and not overthinking it past that.
This test should really move into a web platform test, if there isn't one already, where this approach will work but "remove it from the cache" certainly will not.
Flags: needinfo?(bzbarsky)
![]() |
||
Comment 2•8 years ago
|
||
Oh, and I can move this to a web platform test myself if you'd rather not spend time on it. Just let me know.
Assignee | ||
Comment 3•8 years ago
|
||
it looks like there is an existing wpt test that covers this:
https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/html/semantics/embedded-content/the-img-element/img.complete.html?q=img.complete.html&redirect_type=direct
:bz, can you verify this exists and I will happily delete this mochitest one.
![]() |
||
Comment 4•8 years ago
|
||
That test doesn't test all the same things as our existing test does. In particular, it doesn't check that if you start with an already-loaded image that is complete then setting a new src will immediately set complete to false....
Updated•8 years ago
|
Priority: -- → P3
Assignee | ||
Comment 5•8 years ago
|
||
ok, my first attempt at a wpt test, this is pretty much a blind copy of the existing test. I tested this locally and will push to try if this seems good.
![]() |
||
Comment 6•8 years ago
|
||
You need to do an async_test here, finishing it when the async work is done....
Assignee | ||
Comment 7•8 years ago
|
||
Comment on attachment 8810551 [details]
migrate to web-platform-test
thanks :bz, I will use async_test :)
Attachment #8810551 -
Flags: review?(james)
Comment 8•8 years ago
|
||
Actually you don't have to; without test() or async_test() anywhere on the page it's interpreted as a single page test. But you are calling done() at the wrong time; the test isn't complete until the second image load event fires.
Assignee | ||
Comment 9•8 years ago
|
||
I looked at the test, initially I wanted to call done() in the img.onload handler, but what if the img doesn't load, then the test times out and we fail slowly? Possibly that is the best route for displaying an error- :jgraham, how does wpt handle timeouts if it doesn't receive a done() message?
as a note, I run the test locally with:
./mach web-platform-tests testing/web-platform/tests/html/semantics/embedded-content/the-img-element/update-src-complete.html
and moving done() to the img.onload handler works as expected
Flags: needinfo?(james)
Comment 10•8 years ago
|
||
If the image doesn't load we will indeed time out slowly; after 10s (in default settings) the harness will get status TIMEOUT which will be displayed to the user (or recorded in the test results in CI). In general I don't think you should worry too much about this case because it's pretty unlikely that we totally break image loading.
Flags: needinfo?(james)
Assignee | ||
Comment 11•8 years ago
|
||
thanks for your help so far :jgraham, I know this is a lot of back and forth, learning the small details of wpt tests. I see :bz is still busy, so asking you for review.
Attachment #8810551 -
Attachment is obsolete: true
Attachment #8810945 -
Flags: review?(james)
Comment 12•8 years ago
|
||
Comment on attachment 8810945 [details] [diff] [review]
migrate to web-platform-test
Review of attachment 8810945 [details] [diff] [review]:
-----------------------------------------------------------------
::: testing/web-platform/tests/html/semantics/embedded-content/the-img-element/update-src-complete.html
@@ +15,5 @@
> + done();
> + }
> + }
> +
> + onload = function () {
fwiw I would just have written
onload = function() {
// body of check goes here
}
but this way works too.
Attachment #8810945 -
Flags: review?(james) → review+
Comment 13•8 years ago
|
||
Pushed by jmaher@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/55fe392732ac
migrate dom/html/test/test_img_complete.html to web-platform-test. r=jgraham
Comment 14•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment 15•8 years ago
|
||
bugherder uplift |
status-firefox52:
--- → fixed
Flags: in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•