1316629 - dom/html/test/test_img_complete.html fails to run >1 time in a browser session

Status: RESOLVED FIXED

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

Description

[Joel Maher ( :jmaher ) (UTC -8)]

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?

Comment 1

[Boris Zbarsky [:bzbarsky]]

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.

Comment 2

[Boris Zbarsky [:bzbarsky]]

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.

Comment 3

[Joel Maher ( :jmaher ) (UTC -8)]

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

[Boris Zbarsky [:bzbarsky]]

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....

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: migrate to web-platform-test

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

[Boris Zbarsky [:bzbarsky]]

You need to do an async_test here, finishing it when the async work is done....

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8810551 [details]
migrate to web-platform-test

thanks :bz, I will use async_test :)

Comment 8

[James Graham [:jgraham]]

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.

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

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

Comment 10

[James Graham [:jgraham]]

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.

Comment 11

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: migrate to web-platform-test

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.

Comment 12

[James Graham [:jgraham]]

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.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/55fe392732ac

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8f8be75d4cc5