1902032 - Ensure applicable tabbrowser tests also run with vertical -tabs enabled

Status: RESOLVED FIXED

(Firefox :: Sidebar, task)

Description

[Sarah Clements [:sclements] | PTO until July 14th]

I think we can refactor some tests to run with both verticalTabs pref on and the default (off) rather than trying to writing all new tests for this existing functionality - this would be more around interations like mute/sound playing, etc.

Comment 1

[Sarah Clements [:sclements] | PTO until July 14th]

Looks like we can create an alternative toml file with the tests we want to run with the sidebar.verticalTabs=true and then register that in the manifest like so: https://searchfox.org/mozilla-central/rev/4582d908c17fbf7924f5699609fe4a12c28ddc4a/dom/cache/moz.build#89-92. So we can start with important ones and gradually work through others that require refactoring.

Comment 2

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1902032 - Add test variant to run select tabbrowser tests with sidebar.verticalTabs enabled. r?#tabbrowser-reviewers!,#sidebar-reviewers,jmaher

Comment 3

[Sam Foster [:sfoster] (he/him)]

Can you give the attached WIP patch a look over :aryx and :jmaher? When vertical tabs is enabled, we move the tabstrip to the sidebar and its not a given that all the things we expect to work with a horizontal tabstrip necessarily work in the vertical configuration. We have some test coverage for vertical tabs in browser/components/sidebar, but there's a trove of detailed tests, gotchas and expectations in the browser/components/tabbrowser/test/browser/tabs suite - which has been built up over the years. At a high level, we want those tests to pass in both configurations, but there are some expected differences and a lot of those tests a really about <xul:tab> and <xul:browser> not so much tabstrip or tabbed browser behavior.

So my approach here is to create a new manifest with dupe-manifest = true in the tabs directory, set the pref sidebar.verticalTabs = true and selectively include a subset of those tests to run with this configuration. This gives us apples to apples results, and also lets us skip tests that are currently failing with the pref enabled and file bugs and incrementally re-enable them. And it lets me not have to refactor or copy/paste ~100 test files, with all the risk of bitrot and maintenance headache that implies.

However, this also needs to fit into your workflow as sheriffs and not create more problems than it solves. Is it sufficiently clear when a test fails, which manifest and associated pref values it failed under? And will it be clear enough to everyone involved how and why it failed, how to reproduce the failure etc.?

Comment 4

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

An identic test name cannot be identified as running with a different configuration if it runs within the same task type, e.g. mochitest browser-chrome, shown as M(bc) in Treeherder.

The recommend way to satisfy the needs:

• Add tag = "vertical-tabs" to each test manifest or below the test in the manifest (example)
• Add a boolean test suite property which represents if vertical tabs are enabled or not , similar to these. It will be used to disable tests only in vertical tabs configuration.
• • Add a test variant which will run the tagged tests with the variables set which enable vertical tabs, similar to these. Suggestion for suffix: vt

Further information:

• By default, CI failures will be classified for both horizontal and vertical tabs into one bug.
• There are scripts available to have burndown charts for the disabled tests.

Comment 5

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

there are other examples of duplicated tests with preferences, and we support this in our CI- but when something goes wrong, we usually skip the problem with a large hammer. It is also confusing 2 years later when a new engineer is debugging a test failure and then realizes they spent hours looking at the wrong runtime mode.

I am a big advocate for test-variants as Aryx mentioned ^. This allows reuse of the same test, but then we have a key so we can do:
skip-if = os ... and vertical_tabs or skip-if = os ... and not vertical_tabs

One quirk with variants is that they survive for 6 months, and then need to be renewed. Typically variants were designed for new features coming into the browser that take longer to implement, rollout, etc.; There are some variants with expires: never, after a few rounds of this we know if we need to support both default and variant long term (no future plans for development because both modes have value) and at this point we switch to a expires: never

Comment 6

[Sam Foster [:sfoster] (he/him)]

(In reply to Joel Maher ( :jmaher ) (UTC -8) from comment #5)

I am a big advocate for test-variants as Aryx mentioned ^. This allows reuse of the same test, but then we have a key so we can do:
skip-if = os ... and vertical_tabs or skip-if = os ... and not vertical_tabs

Ok, I'm sold on this approach and I think I understand broadly what the variant does/can do. I think really want to be able to explicitly opt-in tests (or maybe test suites). So I see examples in there for how to ensure the variant only runs for say "mochitest-browser-chrome" == task["try-name"] but it makes little sense to run most of our bc tests in this configuration so rather than adding skip-if to every browser-chrome test, I'd like to restrict to the specific files I'm currently identifying in the browser-verticalTabs.toml (and potentially others down the road) Can I add a tag to a test in the existing browser.toml and have the when clause from my variant definition only return true when the test has that tag? Is that what the MOZHARNESS_TEST_TAG does. E.g?

    merge:
        worker:
            env:
                MOZHARNESS_TEST_TAG: ["vertical-tabs-applicable"]

(I did try this quickly, but I'm either doing it wrong or it doesnt do what I think it should)

One quirk with variants is that they survive for 6 months, and then need to be renewed.

That should be fine. We can review in 6 months and see if this approach is still providing value.

Comment 7

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

yes, using tags is a great way to filter this down. you can see that in media-engine, and there are tags in a few manifests.

Looking forward to seeing these go live soon!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b07b6247a0fd
Add test variant to run select tabbrowser tests with sidebar.verticalTabs enabled. r=jmaher,sidebar-reviewers,tabbrowser-reviewers,taskgraph-reviewers,ahal,nsharpley

Comment 9

[Norisz Fay [:noriszfay]]

Backed out for causing python failure on test_new_config.py

Backout link

Push with failures

Failure log

Comment 10

[Sam Foster [:sfoster] (he/him)]

Thanks for the backout. Looks like the filter function in test_new_config needs an update to expect the new variant tasks.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bca9e1e0626a
Add test variant to run select tabbrowser tests with sidebar.verticalTabs enabled. r=jmaher,sidebar-reviewers,tabbrowser-reviewers,taskgraph-reviewers,ahal,nsharpley

Comment 12

[agoloman]

Backed out for causing bc jobs to not run any tests.

• Backout link
• Push with failures
• Failure Log
  ————————————————————————————————————
• Push with failures bc failures @browser_tabCloseProbes.js
• Failure Log

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ae226b670402
Add test variant to run select tabbrowser tests with sidebar.verticalTabs enabled. r=jmaher,sidebar-reviewers,tabbrowser-reviewers,taskgraph-reviewers,ahal,nsharpley

Comment 14

[Sandor Molnar[:smolnar]]

Backed out for causing py3 unit test failures

Backout link: https://hg.mozilla.org/integration/autoland/rev/cace8a1ed9072b273cbe760edd499b57d1d8f8a3

Push with failures

Failure log ->

L.E
Also these bc-vt

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f7abae721bae
Add test variant to run select tabbrowser tests with sidebar.verticalTabs enabled. r=jmaher,sidebar-reviewers,tabbrowser-reviewers,taskgraph-reviewers,ahal,nsharpley

Comment 16

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/f7abae721bae

Comment 17

[Sam Foster [:sfoster] (he/him)]

Clearing need-info. Thank you for your patience getting this landed.