Open Bug 1322497 Opened 3 years ago Updated 2 years ago

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

Categories

(Core :: Layout, defect, P3)

defect

Tracking

()

ASSIGNED

People

(Reporter: malayaleecoder, Assigned: malayaleecoder)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

./mach mochitest layout/generic/test/test_intrinsic_size_on_loading.html --repeat 1 fails.
Attached patch Bug1322497_v1.diff (obsolete) — Splinter Review
Attachment #8817418 - Flags: review?(jmaher)
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 :)
Assignee: nobody → malayaleecoder
Status: NEW → ASSIGNED
Blocks: 1316113
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?
Attachment #8817418 - Flags: review?(jmaher) → review-
Have a look :)
Attachment #8817418 - Attachment is obsolete: true
Attachment #8817451 - Flags: review?(jmaher)
Attachment #8817451 - Flags: review?(VYV03354)
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.
Attachment #8817451 - Flags: review?(jmaher) → review+
Blocks: 1205027
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.
Attachment #8817451 - Flags: review?(VYV03354) → review-
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.