1436606 - Remove stylo_disabled talos test

Status: RESOLVED FIXED

(Testing :: Talos, enhancement)

Description

[Xidorn Quan [:xidorn] UTC+11]

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.

Comment 1

[Joel Maher ( :jmaher ) (UTC -8)]

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.

Comment 2

[Chris Peterson [: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).

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

(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...

Comment 4

[Chris Peterson [:cpeterson]]

(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.

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: remove talos stylo disabled from trunk

and a try run for your entertainment:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=74e2da7d6e9554e4e80514d0b25c4ee2c0f25a15

Comment 6

[Robert Wood [: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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/643e7c391208
Remove stylo_disabled talos test. r=rwood

Comment 8

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/643e7c391208

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

(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.