1348918 - Add warnings to browser/base/content/test/general/browser.ini because people keep adding tests to it

Status: RESOLVED FIXED

(Firefox :: General, enhancement)

Description

[:Gijs (he/him)]

See https://groups.google.com/forum/?fromgroups=&hl=en#!topic/firefox-dev/sAnDQWlFYU0

Apparently push hooks are frowned upon, so I'd like to go ahead and add annoying comments instead.

Comment 1

[:Gijs (he/him)]

I would have preferred not to do this but I ran into bug 1303838 today, and so now I feel like we should just go ahead and do this in the hope it'll finally stop people.

Comment 2

[:Gijs (he/him)]

Attachment: Bug 1348918 - add stupid comments to browser/base/content/test/general/browser.ini to dissuade people from adding tests,

Review commit: https://reviewboard.mozilla.org/r/122032/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/122032/

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

You can also add Unicode warning symbols "⚠" as in Loader.jsm[1] for an additional scare factor. :)

[1]: searchfox.org/mozilla-central/source/devtools/shared/Loader.jsm

Comment 4

[Dave Townsend [:mossop]]

Comment on attachment 8849204 [details]
Bug 1348918 - add stupid comments to browser/base/content/test/general/browser.ini to dissuade people from adding tests,

https://reviewboard.mozilla.org/r/122032/#review124118

Bleh, ok

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/929c2e196330
add stupid comments to browser/base/content/test/general/browser.ini to dissuade people from adding tests, r=mossop

Comment 6

[Dão Gottwald [::dao]]

Why are we discouraging adding any tests here (i.e. obsoleting general/) rather than moderating it? What's the broader vision here? Bug 1334642 comment 44 suggests adding a subdir for reflow tests (of which I think we have very few). Won't this pattern lead to a future where trying to get an idea whether you've broken basic Firefox UI infrastructure (I believe that's the use case for running tests in general/ locally? If not, what is it?) leaves you with no choice but running all of browser/base/content/test/ unless you have a very good idea what specific area you might have broken?

general/ seems like a useful middle ground between a focused local test run and a Try push, and I'd rather find ways to make this work than break it.

Comment 7

[:Gijs (he/him)]

(In reply to Dão Gottwald [::dao] from comment #6)
> Why are we discouraging adding any tests here (i.e. obsoleting general/)
> rather than moderating it?

Because it's a dumping ground.

> What's the broader vision here?

Not having 260-odd tests in here that take nearly an hour to run in debug builds on infra, with hard-to-understand implicit dependencies between tests.

> Bug 1334642
> comment 44 suggests adding a subdir for reflow tests (of which I think we
> have very few). Won't this pattern lead to a future where trying to get an
> idea whether you've broken basic Firefox UI infrastructure (I believe that's
> the use case for running tests in general/ locally? If not, what is it?)

I didn't think there was a real "usecase" for running this directory right now. If you want to know if you broke UI entirely, start the browser. If you want to know whether you broke tests, you'll need to run more than just general/ anyway (e.g. the customization tests, or the private browsing tests, or the click-to-play tests, or the urlbar tests, or the add-on manager tests, or ... - they all live in other places already).

> leaves you with no choice but running all of browser/base/content/test/
> unless you have a very good idea what specific area you might have broken?

I don't know of a lot of cases where just running general/ is necessary and sufficient for determining whether you're breaking something. Generally, I try to run topical tests locally and then do a trypush if I'm unsure. general/ undermines the concept of running topical tests because, well, it has bits of everything, from popup-blocking to reflow checks to tabbrowser tests to tests that aren't even really testing "Firefox UI" behaviour but whether e.g. tab-switching based on anchor navigation in named browsing contexts works (which is mostly based on DOM/docshell work).

Can you give examples of some bugs where you find/found/would have found this type of test-run useful? I'm struggling to imagine what they would be like.

> general/ seems like a useful middle ground between a focused local test run
> and a Try push, and I'd rather find ways to make this work than break it.

It sounds like what you want is a set of smoketests for common browser UI interactions. That seems worthwhile to me, but I think it would make more sense to create a specific set of tests (in a specific directory, e.g. called "smoketests") that do that. I wonder to what degree the non-tier-1 firefox-ui tests currently provide this.

Comment 8

[Dão Gottwald [::dao]]

(In reply to :Gijs from comment #7)
> (In reply to Dão Gottwald [::dao] from comment #6)
> > Why are we discouraging adding any tests here (i.e. obsoleting general/)
> > rather than moderating it?
> 
> Because it's a dumping ground.

We should moderate it better then. How is making people create subdirs for a test that doesn't fit elsewhere any better except for the infra problem?

> > What's the broader vision here?
> 
> Not having 260-odd tests in here that take nearly an hour to run in debug
> builds on infra,

Can we just have a limit per directory and move from general-x to general-[x+1] when needed?

> with hard-to-understand implicit dependencies between tests.

This is not an effective solution to that. Dependencies between tests need to be prevented by other means. (A few years ago we added some magic to browser-test.js which I think helped a lot but clearly wasn't sufficient.)

> > Bug 1334642
> > comment 44 suggests adding a subdir for reflow tests (of which I think we
> > have very few). Won't this pattern lead to a future where trying to get an
> > idea whether you've broken basic Firefox UI infrastructure (I believe that's
> > the use case for running tests in general/ locally? If not, what is it?)
> 
> I didn't think there was a real "usecase" for running this directory right
> now. If you want to know if you broke UI entirely, start the browser. If you
> want to know whether you broke tests, you'll need to run more than just
> general/ anyway (e.g. the customization tests, or the private browsing
> tests, or the click-to-play tests, or the urlbar tests, or the add-on
> manager tests, or ... - they all live in other places already).
> 
> > leaves you with no choice but running all of browser/base/content/test/
> > unless you have a very good idea what specific area you might have broken?
> 
> I don't know of a lot of cases where just running general/ is necessary and
> sufficient for determining whether you're breaking something.

I said "get an idea" precisely for this reason... E.g. if you hack a on a central piece like the urlbar, running a large bunch of UI tests that interact with the urlbar in weird ways is likely more useful than running a handfull of tests in urlbar/.

If you find a problem in general/, you can spare the Try run. If you don't, obviously this doesn't mean your code is perfect.

Comment 9

[Dave Townsend [:mossop]]

I don't see this as a long-term restriction. A general directory could make sense for tests that don't fit anywhere else but for now it is too big and we have to reduce it. At some point someone needs to go through and just move out a lot of these tests to their own directories then we could open this up. Does anyone have the time for that?

Comment 10

[:Gijs (he/him)]

(In reply to Dão Gottwald [::dao] from comment #8)
> > > What's the broader vision here?
> > 
> > Not having 260-odd tests in here that take nearly an hour to run in debug
> > builds on infra,
> 
> Can we just have a limit per directory and move from general-x to
> general-[x+1] when needed?

This would solve a number of problems, yes, though I don't understand how this is better for your stated usecase - you'll end up having to run general-*, which isn't really significantly different from just running everything under browser/base/content/test/ . As it is, there are about 450 browser tests in all of browser/base/content/test/ . Feels like running all of them isn't actually that different from running all of general/ at the moment (ie no order of magnitude difference, whereas per-feature testing is usually 1-2 orders of magnitude different in terms of the number of tests to run). If we're going to use multiple directories, trying to have more topical directories seems more useful than just "general-5" or whatever...

> > with hard-to-understand implicit dependencies between tests.
> 
> This is not an effective solution to that. Dependencies between tests need
> to be prevented by other means. (A few years ago we added some magic to
> browser-test.js which I think helped a lot but clearly wasn't sufficient.)

Part of the "magic" is run-by-dir, which is exactly what using more subdirs takes advantage of. There's also a shuffle mode (ie run tests within the same dir in a random order) but we don't use this on infra, so in practice lots of tests will go orange if you use it. It would be a good idea to investigate using it on infra, IMO, as it'll help detect flakey tests/code. Not sure how much work doing that is, though.

> > > leaves you with no choice but running all of browser/base/content/test/
> > > unless you have a very good idea what specific area you might have broken?
> > 
> > I don't know of a lot of cases where just running general/ is necessary and
> > sufficient for determining whether you're breaking something.
> 
> I said "get an idea" precisely for this reason... E.g. if you hack a on a
> central piece like the urlbar, running a large bunch of UI tests that
> interact with the urlbar in weird ways is likely more useful than running a
> handfull of tests in urlbar/.

urlbar/ has 60-odd tests. It's probably the largest folder next to general/. Only 23 tests in general/ actually contain 'gURLBar' (26 case-insensitive hits for 'urlbar'), much fewer actually assert anything about it (and those might want to move to urlbar/ anyway). I'm skeptical running general/ gives you more information than running urlbar/ when changing the url bar. Of course, it might depend on what you're doing to the URL bar...

(In reply to Dave Townsend [:mossop] from comment #9)
> At some point someone needs to go through and just
> move out a lot of these tests to their own directories then we could open
> this up. Does anyone have the time for that?

Not a lot. I'll try to make a bit of a dent in my spare time. An easy target would be the tracking protection tests, which are tagged and might not even want to be in browser/base/content/test/ at all. We could also see if there's a significant chunk of tabbrowser tests that we could reasonably move to the extant tabs directory. Part of the problem is that there's usually new test failures when trying to do this. The other problems include the use of [default] for support-files (so not obvious which tests need which files) and excerpting the relevant bits out of head.js (or replacing their use with BTU equivalents) without breaking the rest of the tests.

Comment 11

[Dão Gottwald [::dao]]

(In reply to :Gijs from comment #10)
> (In reply to Dão Gottwald [::dao] from comment #8)
> > Can we just have a limit per directory and move from general-x to
> > general-[x+1] when needed?
> 
> This would solve a number of problems, yes, though I don't understand how
> this is better for your stated usecase - you'll end up having to run
> general-*, which isn't really significantly different from just running
> everything under browser/base/content/test/ . As it is, there are about 450
> browser tests in all of browser/base/content/test/ . Feels like running all
> of them isn't actually that different from running all of general/ at the
> moment (ie no order of magnitude difference, whereas per-feature testing is
> usually 1-2 orders of magnitude different in terms of the number of tests to
> run).

The division into subdirectories is relatively new in browser/base/content/test/, so it's actually encouraging that the rest is already as big as general/. It seems safe to assume that general will continue to grow slower than the rest, especially if we do a better job of moderating it. So yes, I still think running only general/ locally is a trade-off that can continue to make sense, and if somebody feels like running all of browser/base/content/test/, they have that option too. It doesn't need to be either-or.

> If we're going to use multiple directories, trying to have more
> topical directories seems more useful than just "general-5" or whatever...

I'm not arguing against doing that where it makes sense. It may however not always make sense, see bug 1334642.

> > > with hard-to-understand implicit dependencies between tests.
> > 
> > This is not an effective solution to that. Dependencies between tests need
> > to be prevented by other means. (A few years ago we added some magic to
> > browser-test.js which I think helped a lot but clearly wasn't sufficient.)
> 
> Part of the "magic" is run-by-dir, which is exactly what using more subdirs
> takes advantage of.

No, I meant older efforts like blaming tests for leaving tabs behind or adding stuff to the global scope. It's pretty primitive stuff and could probably be improved with some platform support.

> > I said "get an idea" precisely for this reason... E.g. if you hack a on a
> > central piece like the urlbar, running a large bunch of UI tests that
> > interact with the urlbar in weird ways is likely more useful than running a
> > handfull of tests in urlbar/.
> 
> urlbar/ has 60-odd tests. It's probably the largest folder next to general/.
> Only 23 tests in general/ actually contain 'gURLBar' (26 case-insensitive
> hits for 'urlbar'), much fewer actually assert anything about it (and those
> might want to move to urlbar/ anyway). I'm skeptical running general/ gives
> you more information than running urlbar/ when changing the url bar. Of
> course, it might depend on what you're doing to the URL bar...

Tests can (and pretty much all browser tests do, although often in trivial ways) stress the urlbar without explicitly using gURLBar. Anyway, this isn't about the urlbar, which I just used as a placeholder to get an idea across. Good to hear that urlbar/ has 60 tests already, I expected fewer.

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/929c2e196330