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

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
5 years ago
4 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

5 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

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/69615044a960
Whiteboard: [tests disabled on !Windows][leave open]
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

5 years ago
Posted patch test.patch (obsolete) — Splinter Review
The dependence is fixed. Can we try to reenable the tests?

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

Updated

5 years ago
Depends on: 1042908
Assignee

Comment 6

5 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

5 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

5 years ago
Looks like there's a new B2G debug intermittent as well:
https://tbpl.mozilla.org/php/getParsedLog.php?id=45534564&tree=Try
(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)
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

5 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

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

Comment 13

5 years ago
Posted patch test1.patch (obsolete) — Splinter Review
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

5 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

5 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

5 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

5 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]
Assignee

Comment 19

5 years ago
Posted patch test1.patch (obsolete) — Splinter Review
This excludes b2g as well.
Attachment #8476597 - Attachment is obsolete: true
Attachment #8478336 - Flags: review?(ryanvm)
Assignee

Updated

5 years ago
Assignee: nsm.nikhil → amarchesini
Reporter

Comment 20

5 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

5 years ago
Posted patch test1.patchSplinter Review
Attachment #8478336 - Attachment is obsolete: true
Attachment #8478355 - Flags: review?(ryanvm)
Reporter

Updated

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

Updated

5 years ago
Blocks: 1056702
(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)
We already have these enabled on central for everything except b2g. Bug 1053275 will fix intermittents.
Status: NEW → RESOLVED
Closed: 4 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.