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

ASSIGNED
Assigned to

Status

()

P3
normal
ASSIGNED
2 years ago
a year ago

People

(Reporter: malayaleecoder, Assigned: malayaleecoder)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
./mach mochitest layout/generic/test/test_intrinsic_size_on_loading.html --repeat 1 fails.
(Assignee)

Comment 1

2 years ago
Created attachment 8817418 [details] [diff] [review]
Bug1322497_v1.diff
Attachment #8817418 - Flags: review?(jmaher)
(Assignee)

Comment 2

2 years ago
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
(Assignee)

Updated

2 years ago
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-
(Assignee)

Comment 4

2 years ago
Created attachment 8817451 [details] [diff] [review]
Bug1322497_v2.diff

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.