Closed Bug 1436606 Opened 4 years ago Closed 4 years ago

Remove stylo_disabled talos test

Categories

(Testing :: Talos, enhancement)

enhancement
Not set
normal

Tracking

(firefox58 wontfix, firefox59 wontfix, firefox60 fixed)

RESOLVED FIXED
mozilla60
Tracking Status
firefox58 --- wontfix
firefox59 --- wontfix
firefox60 --- fixed

People

(Reporter: xidorn, Assigned: jmaher)

References

Details

(Whiteboard: [PI:February])

Attachments

(1 file)

According to talos result, once we enable stylo-chrome, result of stylo_disabled talos tests on pgo build would be regressed significantly.

This is probably because the old style system is now completely disabled by default, and thus not trained by pgo. But we don't really care about stylo_disabled anymore since the old style system is sunsetting. We can probably just drop those tests.
I was under the impression we needed to wait for v.62 to hit trunk when we have shipped v.60- I am happy to disable stylo-disabled talos tests now on trunk though.
Flags: needinfo?(cpeterson)
(In reply to Joel Maher ( :jmaher) (UTC-5) from comment #1)
> I was under the impression we needed to wait for v.62 to hit trunk when we
> have shipped v.60- I am happy to disable stylo-disabled talos tests now on
> trunk though.

Sure, we can disable the stylo-disabled *Talos* tests now.

We want to keep running the stylo-disabled unit tests until Stylo-android hits the Release channel (hopefully in Firefox 60 on May 8).
Flags: needinfo?(cpeterson)
(In reply to Chris Peterson [:cpeterson] from comment #2)
> (In reply to Joel Maher ( :jmaher) (UTC-5) from comment #1)
> > I was under the impression we needed to wait for v.62 to hit trunk when we
> > have shipped v.60- I am happy to disable stylo-disabled talos tests now on
> > trunk though.
> 
> Sure, we can disable the stylo-disabled *Talos* tests now.
> 
> We want to keep running the stylo-disabled unit tests until Stylo-android
> hits the Release channel (hopefully in Firefox 60 on May 8).

We probably don't want that either, because that means we cannot remove the old style system in 60esr, and may have to fix security issues in it for another one year or so. Maybe it's not that bad, though, but I suspect...
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #3)
> We probably don't want that either, because that means we cannot remove the
> old style system in 60esr, and may have to fix security issues in it for
> another one year or so. Maybe it's not that bad, though, but I suspect...

Why would having the old style system code in 60esr be an extra burden for security issues if Stylo is enabled by default? Because downstream distros might disable Stylo in their Firefox builds? We don't need to run the stylo-disabled tests after we branch 60esr.
Whiteboard: [PI:February]
and a try run for your entertainment:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=74e2da7d6e9554e4e80514d0b25c4ee2c0f25a15
Assignee: nobody → jmaher
Status: NEW → ASSIGNED
Attachment #8949365 - Flags: review?(rwood)
Comment on attachment 8949365 [details] [diff] [review]
remove talos stylo disabled from trunk

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

Missed a couple of entries in 'talos.yml' but otherwise looks great! Another bunch of resources set free! :)

::: taskcluster/ci/test/talos.yml
@@ -244,5 @@
>              - --suite=g3
>              - --geckoProfile
>              - --add-option
>              - --webServer,localhost
>  

Missed talos-g1-stylo-disabled and talos-g2-stylo-disabled

@@ -339,5 @@
>      max-run-time:
>          by-test-platform:
>              linux64.*: 1200
>              default: 1800
>  

Missed talos-g4-stylo-disabled
Attachment #8949365 - Flags: review?(rwood) → review+
Pushed by jmaher@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/643e7c391208
Remove stylo_disabled talos test. r=rwood
https://hg.mozilla.org/mozilla-central/rev/643e7c391208
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
(In reply to Chris Peterson [:cpeterson] from comment #4)
> (In reply to Xidorn Quan [:xidorn] UTC+10 from comment #3)
> > We probably don't want that either, because that means we cannot remove the
> > old style system in 60esr, and may have to fix security issues in it for
> > another one year or so. Maybe it's not that bad, though, but I suspect...
> 
> Why would having the old style system code in 60esr be an extra burden for
> security issues if Stylo is enabled by default? Because downstream distros
> might disable Stylo in their Firefox builds? We don't need to run the
> stylo-disabled tests after we branch 60esr.

Well... It's probably not just a maintenance burden on security issue in the old style system code. There is likely to be more refactor coming once the old style system gets removed (e.g. simplifying lots of dispatching code, merging the classes), which may change larger portion of style system code, and make uplifting harder.

But given that we haven't enabled stylo-chrome yet, it is unclear whether it is possible to get to those simplifications even if we can remove the old style system code before 60beta branches. So maybe that's something we have to live with.
Duplicate of this bug: 1432704
You need to log in before you can comment on or make changes to this bug.