1322497 - layout/generic/test/test_intrinsic_size_on_loading.html fails to run more than once in same browser session

Status: NEW

(Core :: Layout, defect, P3)

Description

[Vishnu (:malayaleecoder)]

./mach mochitest layout/generic/test/test_intrinsic_size_on_loading.html --repeat 1 fails.

Comment 1

[Vishnu (:malayaleecoder)]

Attachment: Bug1322497_v1.diff

Comment 2

[Vishnu (:malayaleecoder)]

This fix manages to avoid the error happening for multiple runs, but not a logical way. I tried removing the 8 loaded images just before finish, but on the successive run, initialOffsetTop takes the value of "8". Do let me know if there is a logical solution :)

Comment 3

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

Comment on attachment 8817418 [details] [diff] [review]
Bug1322497_v1.diff

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

really close, lets do a few small cleanup items and get this back up for review!  Thanks for working on this.

::: layout/generic/test/test_intrinsic_size_on_loading.html
@@ +45,5 @@
>    <script>
>      initialOffsetTop = marker.offsetTop;
> +    // Adjust the value of initialOffsetTop when running multiple times in same session
> +    if (initialOffsetTop == 8)
> +    	initialOffsetTop = 0;

A few small nits here-
1) the comment is too long, it should be split on multiple lines
2) in the future we will question why 8 is a magic number, that is just room for future failures- can we at least make it a const, or determine a way to reset when we finish or start the test?
3) I would prefer a initialize() or cleanup() method that we call, although for this case that would seem unecessary.

good job on finding this, I had looked at this unsuccessfully the other day.  When you update this, can you ask :emk for review?

Comment 4

[Vishnu (:malayaleecoder)]

Attachment: Bug1322497_v2.diff

Have a look :)

Comment 5

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

Comment on attachment 8817451 [details] [diff] [review]
Bug1322497_v2.diff

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

I am happier with this- lets see if :emk has other feedback.

Comment 6

[Masatoshi Kimura [:emk]]

Comment on attachment 8817451 [details] [diff] [review]
Bug1322497_v2.diff

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

If this test is repeated, it does not test what it is supposed to test. As written in the manual test case in bug 1205027, this test does not work if the image is already cached.

To make the test repeatable, you will have to change file_SlowImage.sjs to
1. make images uncacheable and
2. add a "command" (like "?continue") to reinitialize the promise.

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.