Fix and re-enable the serviceworker tests on non-Windows

RESOLVED FIXED

Status

()

Core
DOM: Workers
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: RyanVM, Assigned: baku)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

3 years ago
Per this bug's deps, the serviceworker tests are unreliable on every platform except Windows. Due to their ongoing issues and the upcoming merge, they are being disabled on affected platforms. This bug tracks fixing and re-enabling them.
(Reporter)

Comment 1

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/69615044a960
Whiteboard: [tests disabled on !Windows][leave open]
https://hg.mozilla.org/mozilla-central/rev/69615044a960

Updated

3 years ago
Blocks: 903441
Depends on: 1047398
Hey bent --- is the cause of this timeout known already?  It's not clear from the context in bug 1037739.

p.s. I'd like to use rr to debug this ;).
Flags: needinfo?(bent.mozilla)
Chris, I think our best theory was bug 1042908. Maybe apply that first and then see if rr shows anything? Thanks!
Flags: needinfo?(bent.mozilla) → needinfo?(cjones.bugs)
(Assignee)

Comment 5

3 years ago
Created attachment 8468455 [details] [diff] [review]
test.patch

The dependence is fixed. Can we try to reenable the tests?

https://tbpl.mozilla.org/?tree=Try&rev=b0b3e94441af
(Reporter)

Updated

3 years ago
Depends on: 1042908
(Assignee)

Comment 6

3 years ago
This is the latest result: https://tbpl.mozilla.org/?tree=Try&rev=5d676000e336

To me we can re-enable it and fix the issue when we find it.
Flags: needinfo?(ryanvm)
Flags: needinfo?(bkelly)
(Reporter)

Comment 7

3 years ago
Looks like Android 2.3 is still in pretty bad shape. I'd support re-enabling everywhere else, though. Changing the annotation to |skip-if = android_version == "10"| should do the trick. Would also be nice if bug 1042908 could be completed in nsm's absence.
Flags: needinfo?(ryanvm)
(Reporter)

Comment 8

3 years ago
Looks like there's a new B2G debug intermittent as well:
https://tbpl.mozilla.org/php/getParsedLog.php?id=45534564&tree=Try

Comment 9

3 years ago
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #7)
> Looks like Android 2.3 is still in pretty bad shape. I'd support re-enabling
> everywhere else, though. Changing the annotation to |skip-if =
> android_version == "10"| should do the trick. Would also be nice if bug
> 1042908 could be completed in nsm's absence.

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #8)
> Looks like there's a new B2G debug intermittent as well:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=45534564&tree=Try

Since the code is going to be changing a lot over the next few weeks I'm not sure its worth hunting these down right now.  There is a good chance they will get fixed as we finish implementing the rest of SW.

So lets just skip on android and b2g-emulator.  Please just file a follow-up bug to re-enable and hang it off bug 903441.
Flags: needinfo?(bkelly)

Comment 10

3 years ago
Just to clarify, I don't think its worth hunting down given how slow it is to investigate android / b2g-emulator issues remotely via try.  We should definitely hunt down any desktop intermittents, though.
(Reporter)

Comment 11

3 years ago
Trying to keep things as targeted as possible, let's go with:
skip-if = android_version == "10" || (toolkit == "gonk" && debug) # <new bug for re-enabling>

We can always get more restrictive later if we find more intermittents occurring in production. Andrea, mind filing said bug and updating the patch? :)
Flags: needinfo?(amarchesini)
(Assignee)

Comment 12

3 years ago
Sure. Before doing this I want to see the 'try' result of bug 1056259.
(Assignee)

Comment 13

3 years ago
Created attachment 8476597 [details] [diff] [review]
test1.patch

https://tbpl.mozilla.org/?tree=Try&rev=0389ba661302

Ok, the android implementation is still buggy.
And for b2g I have a separated patch.
Attachment #8468455 - Attachment is obsolete: true
Attachment #8476597 - Flags: review?(ryanvm)
Flags: needinfo?(amarchesini)
(Reporter)

Comment 14

3 years ago
How does the Try push in comment 13 relate to the Try pushes for the other checkin-needed patches of yours I pushed earlier today that had failures you said were OK to ignore?
(Assignee)

Comment 15

3 years ago
Yesterday we landed bug 1056259 and this fixed several failures of this try push: https://tbpl.mozilla.org/?tree=Try&rev=28d7624a47fe

Today the situation was much better, talking about ServiceWorkers.
There are still issues with b2g where window.open() fails, but my solution is still not enough:
https://tbpl.mozilla.org/?tree=Try&rev=ec72ac6fb344

Does it answer to your question?
(Reporter)

Comment 16

3 years ago
Comment on attachment 8476597 [details] [diff] [review]
test1.patch

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

Worth a shot. We can always make it more restrictive later if need-be.
Attachment #8476597 - Flags: review?(ryanvm) → review+
(Reporter)

Comment 17

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/171355fe87e1
Whiteboard: [tests disabled on !Windows][leave open] → [tests disabled on Android 2.3 and B2G debug][leave open]
Reverted for failures on B2G desktop (the try run didn't cover these and the skip-if syntax doesn't exclude B2G desktop):
https://tbpl.mozilla.org/php/getParsedLog.php?id=46560123&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=46560372&tree=Mozilla-Inbound

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2ee31951b96f
(Assignee)

Comment 19

3 years ago
Created attachment 8478336 [details] [diff] [review]
test1.patch

This excludes b2g as well.
Attachment #8476597 - Attachment is obsolete: true
Attachment #8478336 - Flags: review?(ryanvm)
(Assignee)

Updated

3 years ago
Assignee: nsm.nikhil → amarchesini
(Reporter)

Comment 20

3 years ago
Comment on attachment 8478336 [details] [diff] [review]
test1.patch

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

No need for the (toolkit == "gonk" && debug) part if you're skipping all of B2G.
Attachment #8478336 - Flags: review?(ryanvm) → review-
(Assignee)

Comment 21

3 years ago
Created attachment 8478355 [details] [diff] [review]
test1.patch
Attachment #8478336 - Attachment is obsolete: true
Attachment #8478355 - Flags: review?(ryanvm)
(Reporter)

Updated

3 years ago
Attachment #8478355 - Flags: review?(ryanvm) → review+
(Reporter)

Updated

3 years ago
Blocks: 1056702
(Assignee)

Comment 22

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ab548becb6f
https://hg.mozilla.org/mozilla-central/rev/6ab548becb6f
(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #4)
> Chris, I think our best theory was bug 1042908. Maybe apply that first and
> then see if rr shows anything? Thanks!

I was able to repro either this or another of the serviceworker intermittent-fails with the fix from bug 1042908, but I left this open to double-check this particular symptoms.  The odds of that being meaningful are pretty low at this point so clearing request.  Sorry bent.
Flags: needinfo?(cjones.bugs)
Blocks: 1059784
No longer blocks: 903441
We already have these enabled on central for everything except b2g. Bug 1053275 will fix intermittents.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Whiteboard: [tests disabled on Android 2.3 and B2G debug][leave open]
You need to log in before you can comment on or make changes to this bug.