Intermittent dom/base/test/test_intersectionobservers.html | triggers only once if observed multiple times (and does not crash when collected) [observe] - got +0, expected 1

RESOLVED FIXED in Firefox 54

Status

()

defect
RESOLVED FIXED
3 years ago
5 months ago

People

(Reporter: intermittent-bug-filer, Assigned: tschneider)

Tracking

({intermittent-failure})

unspecified
mozilla55
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox53 unaffected, firefox54 fixed, firefox55 fixed)

Details

(Whiteboard: [stockwell fixed])

Attachments

(2 attachments, 1 obsolete attachment)

ni myself to investigate a bit more
Flags: needinfo?(jmaher)
Whiteboard: [stockwell needswork]
Flags: needinfo?(jmaher)
See Also: → 1332922, 1332923, 1354163, 1313970
this was related to bug 1321865, it was backed out as we had a ~perma fail in bug 1313972.
Whiteboard: [stockwell needswork] → [stockwell disabled]
Can this test be rewritten this way to remove the race?

      it('triggers only once if observed multiple times (and does not crash when collected)', function(done) {
        var spy = sinon.spy();
        io = new IntersectionObserver(spy, {root: rootEl});
        io.observe(targetEl1);
        io.observe(targetEl1);
        io.observe(targetEl1);

        spy.waitForNotification(function() {
          callDelayed(function () {
            expect(spy.callCount).to.be(1);
            done();
          }, ASYNC_TIMEOUT);
        }, 0);

      });
Flags: needinfo?(tschneider)
Yes, that would work.
Flags: needinfo?(tschneider)
Posted patch Fix intermittent test failure (obsolete) — Splinter Review
Applies recommended change from Comment 8 plus removes unnecessary delay arguments.
Attachment #8858436 - Flags: review?(bugs)
Comment on attachment 8858436 [details] [diff] [review]
Fix intermittent test failure


>@@ -918,10 +918,12 @@ limitations under the License.
>         io.observe(targetEl1);
>         io.observe(targetEl1);
> 
>-        callDelayed(function () {
>-          expect(spy.callCount).to.be(1);
>-          done();
>-        }, ASYNC_TIMEOUT * 3);
>+        spy.waitForNotification(function() {
>+          callDelayed(function () {
>+            expect(spy.callCount).to.be(1);
>+            done();
>+          }, ASYNC_TIMEOUT);
>+        }, 0);

Lose the 0 here if that's your new convention.

r+ with nits fixed and better commit message. Maybe:

Wait 300ms after the first IntersectionObserver notification to prevent intermittent test failure.
Attachment #8858436 - Flags: review?(bugs) → review+
Keywords: checkin-needed
Pushed by philringnalda@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/940919c5af90
Wait 300ms after the first IntersectionObserver notification to prevent intermittent test failure, r=jet
Keywords: checkin-needed
Assignee: nobody → tschneider
https://hg.mozilla.org/mozilla-central/rev/940919c5af90
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment on attachment 8858491 [details] [diff] [review]
Wait 300ms after the first IntersectionObserver notification to prevent intermittent test failure.

Approval Request Comment
[Feature/Bug causing the regression]: 1321865
[User impact if declined]: No test coverage
[Is this code covered by automated tests?]: It is one
[Has the fix been verified in Nightly?]: Yes
[Is the change risky?]: No
Attachment #8858491 - Flags: approval-mozilla-beta?
Comment on attachment 8858491 [details] [diff] [review]
Wait 300ms after the first IntersectionObserver notification to prevent intermittent test failure.

not part of the build
Attachment #8858491 - Flags: approval-mozilla-beta?
Blocks: 1362168
Whiteboard: [stockwell disabled] → [stockwell disabled] [checkin-needed-beta]
https://hg.mozilla.org/releases/mozilla-beta/rev/b0a7956aa594
Flags: in-testsuite+
Whiteboard: [stockwell disabled] [checkin-needed-beta] → [stockwell fixed]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.