Closed
Bug 1436606
Opened 7 years ago
Closed 7 years ago
Remove stylo_disabled talos test
Categories
(Testing :: Talos, enhancement)
Testing
Talos
Tracking
(firefox58 wontfix, firefox59 wontfix, firefox60 fixed)
RESOLVED
FIXED
mozilla60
People
(Reporter: xidorn, Assigned: jmaher)
References
Details
(Whiteboard: [PI:February])
Attachments
(1 file)
(deleted),
patch
|
rwood
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•7 years ago
|
||
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)
Comment 2•7 years ago
|
||
(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).
status-firefox58:
--- → wontfix
status-firefox59:
--- → wontfix
status-firefox60:
--- → affected
Flags: needinfo?(cpeterson)
Reporter | ||
Comment 3•7 years ago
|
||
(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•7 years ago
|
||
(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.
Assignee | ||
Updated•7 years ago
|
Whiteboard: [PI:February]
Assignee | ||
Comment 5•7 years ago
|
||
and a try run for your entertainment:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=74e2da7d6e9554e4e80514d0b25c4ee2c0f25a15
Comment 6•7 years ago
|
||
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
Comment 8•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Reporter | ||
Comment 9•7 years ago
|
||
(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.
You need to log in
before you can comment on or make changes to this bug.
Description
•