Reorganization of Firefox-UI tests in mozilla-central

Matthew N. MattN at mozilla.com
Fri Sep 2 20:56:02 UTC 2016


On 2016-09-02 2:29 AM, Gijs Kruitbosch wrote:
> On 02/09/2016 00:15, Matthew N. wrote:
>> On 2016-09-01 9:24 AM, Gijs Kruitbosch wrote:
>>> On 01/09/2016 16:37, Henrik Skupin wrote:
>>>> Do those locations sound good? I have heard at least once that
>>>> "firefox_ui" might not be the best choice as folder name, but that's
>>>> how
>>>> the harness is called, and corresponds to what we have for other
>>>> harnesses too.
>>>
>>> As I did over IRC, I would like to strongly object to the continued use
>>> of per-test-type subfolders in our test directories. You can already use
>>> a specific mach command per test type, and the tests are listed in
>>> different manifests, *and* there's all the different filename
>>> conventions (browser_, test_....html, test_....xul, <whatever>.js) that
>>> further point out what type of test you're looking at. The subfolders
>>> add nothing useful.
>>
>> As someone who has been adding the directory levels to
>> toolkit/components/passwordmgr/test/ recently, I disagree with this
>> since they add a grouping of relevant files making it more obvious which
>> files go with which test suites.
>
> But in general you don't need to know this? When does it matter and is
> it an unknown? You can just pass a filename to ./mach test and it'll do
> the right thing.

* refactoring tests and related helper functions (e.g. for e10s). e.g. 
pwmgr_common.js or notification_common.js could belong to any of the 
four suites used by pwmgr. Same for a head.js file.
* when working on a test and the directory doesn't take a super long 
time to run I often want to run everything in the directory for the 
suite of the I'm working on, especially as I'm putting the final touches 
on the test. This is mostly to make sure I'm doing proper cleanup in the 
test.
* knowing which of the ini files to add a new test to (and actually 
noticing the ini files if there are lots of files in the directory).
* when writing a new test I want to look at what helpers relevant to the 
component are available for that suite

>> * Chrome mochitests, plain mochitests and xpcshell share the same prefix
>> (test_) so they are interleaved together in directory listings.
>
> Again, not sure why that's a problem. If there's too many files in a
> directory, that's a problem, but I'd rather we split them out by
> function/subject than by test suite.

See above

>> * Files that accompany tests have no prefix convention.
>
> Yes, but if that is a problem (which I'm not sure about) it continues to
> be a problem when you don't mix test types into one directory.

If you're not mixing then you know that accompanying files are relevant 
to the tests in that directory.

>> * head.js files have no indication of what suite they're for (i.e. no
>> prefix)
>
> But there's no need for the files to be called head.js. All of this is
> also already true without adding the firefox-ui tests.

Right (for xpcshell) but that's the standard name that is commonly used, 
second is to have head_(name of thing being tested).js which would still 
conflict with another head_(name of thing being tested).js for a 
different suite in the same directory if they're testing the same 
module/component.

>> * `mach test` doesn't support specifying a flavor of mochitest.
>
> I'm sure that could be added (in fact, I thought there were plans for
> this already) - in the meantime, ./mach mochitest works.

I would really like this but it's still a reason to not flatten test 
directories at the moment.

>> * Changing the subdirectory of my `mach mochitest` command is usually
>> faster than adding the flavor argument since the path is usually at the
>> end of the command. Since `mach test` doesn't support the flavor
>> argument I don't have to remember whether to use the argument or change
>> the path as I can always change the path when directories are used.
>
> Again, we should just fix ./mach test. But also, this isn't just about
> the mach command, but also about editing, and running individual tests,
> where the subdirectory "system" forces me to do useless extra typing to
> run/edit "browser/browser_foo.js". It gets even worse when you write a
> new test and realize halfway through that you need to switch from
> mochitest-plain to mochitest-browser because mochitest-plain doesn't
> have enough privileges to determine whatever you need to determine.

In the rare cases where you need to move a test between suites I don't 
see this a big deal.

> Out of curiosity: if you're not running a single test, why isn't just
> running all the mochitests sufficient? Why are you wanting to run a
> specific suite but not the others? And is that really the majority case?

My most common experience is with password manager which uses three 
flavors of mochitest (it really should only need 2 but the last "chrome" 
test hasn't been rewritten yet). When I'm developing a new test which 
may involve refactoring shared suite helpers for the directory and I 
already know that my application code change is passing all existing 
test suites I don't want to have to wait for the unrelated mochitest 
flavors to run while developing.

I can't say whether this is the majority case in the long run but it is 
and was while converting tests to work in e10s as that required changing 
a lot of test helpers that are shared with the suite for the directory.

> Personally, I would prefer to have per-subject directories, and to have
> "mach test" and friends allow specifying particular suites if we're
> hunting down other-test-dependent intermittents in a particular suite,
> or something. The most common situation right now is that you want to
> run either a single test or want to ensure that the code changes you
> just made didn't break any of the relevant tests. In the latter case
> "relevant tests" is related to *what the tests are testing*, not to what
> framework the tests use. So ultimately, the grouping by test framework
> makes it hard to run only the tests you care about. I know that some
> test frameworks (last I checked, not including marionette or fx-ui)
> support running tests by "tags", but most tests aren't annotated with
> them, and in any case I feel like it ends up being a hack for the fact
> that our directory layout is not well-suited for that usecase.

It sounds like you're talking more about testing features that don't 
have their own feature directory (e.g. /browser/base/content/test and 
specifically the "general" subdirectory) whereas I'm talking more about 
the opposite (e.g. /toolkit/components/passwordmgr/test). So when you 
talk about "per-subject" directories, that already exists above the 
"test" directory (e.g. "passwordmgr") for my cases where "content" isn't 
really a specific subject in that case of /browser/base/content/test.

Another difference seems to be that you're talking more about feature 
that only use one suite whereas I'm thinking more about ones that use 
multiple suites (e.g. passwordmgr, migration, satchel, etc.). As I said 
in my last email, I think it's fine to not have a sudirectory named 
after the type of test suite if there's only one suite, I think we 
should continue to have subdirectories when there are multiple suites used.

>> * xpcshell and browser-chrome both use "head.js" as the filename for
>> helpers though they run in very different contexts.
>
> xpcshell lets you use whatever you like, I think, with the head/tail
> annotations in the .ini file, for cases where this would actually be
> confusing.

See what I said above about the common pattern being "head.js" or 
"head_(name of thing being tested).js". Sure we could do extra work to 
disambiguate but a directory has the other benefits I mentioned.

>> For a new contributor, having each suite in their own directory is much
>> less confusing/overwhelming for the above reasons.
>
> I will buy that it is a more obvious signal (than filename and content)
> if you don't know much about our testing infrastructure and need to know
> what kind of test you're looking at while you're looking at the
> directory view. But again, that doesn't feel like it's the most common
> case. Wanting to write a new test (where you'd need advice anyway,
> especially given that our naming (chrome/browser-chrome) is so
> confusing) or editing a specific existing test (e.g. one that fails on
> try) don't actually fall in this category. The test prefix and suffix
> are sufficient to know what type of test you're looking at.

They're sufficient once you know (when talking about the test entrypoint 
itself) but I'd rather it be more obvious from the beginning. They're 
still not sufficient for helper scripts and other test-related helper files.

> If we want
> to change conventions there to make it even more obvious, we now can,
> thanks to manifest files, but that feels like a separate discussion.

Or we can keep using directory names to make it obvious…

>> Password Manager may be special in that it's using four different suites
>> so I'm not suggesting that every component needs to put their tests in a
>> subdirectory named after the test suite but I don't think we should be
>> dumping tests of different suites in one directory unless the
>> distinction between files would be clear to a newcomer.
>>
>>> Furthermore, only the toolkit case is currently meaningfully split out
>>> into subdirs. The sessionstore test/ dir has a subdir (but also has a
>>> bundle of tests directly in that dir)
>>
>> Sure, but there isn't a mix of multiple suites in one directory here.
>
> There is in browser/base/content/test/general, dom/base/test/, ... ie it
> is not something that would be unheard of or anything like that.

Sure, I was talking about the specific case of adding puppeteer tests in 
that directory. I believe the dom/ tests were being organized more in 
the last two years and would guess that DOM developers would agree that 
/dom/base/test/ is not ideal.

>>> , and the privatebrowsing one has
>>> no leafs and only a subdir ("browser"). None of the others have any
>>> subdirs at all.
>>
>> That just seems like good planning for the future when other suites are
>> used for that code. The paths of the tests would need to change.
>
> That doesn't feel very likely for most of the browser/ code, though - as
> you note later, we almost exclusively use browser-chrome mochitests,
> only sometimes combined with xpcshell for standalone components (which
> in turn seems unlikely to happen for privatebrowsing because it tends to
> need a DOM infrastructure of some sort).

I think traditionally we haven't been good about making our code unit 
testable but I agree that it depends on the component.

>>> Getting back to the toolkit case, the subdirs are
>>> actually confusing because only some of the subdirs have tests (as a
>>> counterexample, "data" just has random helper files) and the root
>>> testing dir also has .cpp files in it (I guess for gtests?).
>>
>> Nobody is saying that directories under a "test"/"tests" directory
>> should only include test file themselves. Having files to support tests
>> in organized directories doesn't seem like a problem to me.
>
> I think it would make more sense, if we felt that having a dir for
> support files was necessary, to have it as a subdir of wherever the
> tests were, not as a sibling dir.

Except that sharing support files between suites is useful to avoid 
reinventing the wheel and this is used for passwordmgr. I agree the 
support files should be under the test suite directory when they're not 
shared.

>>> IOW, in the general case, I think that in most of those directories
>>> there's no precedent to do what you propose.
>>
>> Having the new subdirectory in these specific cases may not fit but I
>> disagree that it's the wrong approach in general.
>
> Looks like we're not going to be agreeing on this. :-)

Yeah, it seems we have had different experience from working on 
different parts of the code base. :)

>>> Finally, "firefox_ui" (as well as "ui") as a name for a directory is
>>> going to cause all kinds of confusion for people exploring the repo
>>> without detailed knowledge of what's going on. Additionally, it's not
>>> like most of the mochitest-browser tests aren't tests of the Firefox
>>> UI... If we absolutely must have some kind of subdirectory because of
>>> reasons I have yet to hear, I think "integration" would be a better
>>> choice of name as far as subdirs of "test" go (as juxtaposed to "unit"
>>> for our xpcshell tests).
>>
>> "firefox_ui", "ui", and "integration" all overlap with what
>> mochitest-browser-chrome is about IMO and I think naming the suite
>> "Firefox-UI" was confusing from the beginning. If I was a new
>> contributor working on a UI feature and decided I wanted to write tests,
>> I wouldn't want to be misled into thinking I should write a "Firefox-UI"
>> test when mochitest-browser-chrome is actually the desired suite. I
>> would suggest "puppeteer" or "marionette" for directory names to avoid
>> confusion.
>
> If these tests do end up in subdirs, I agree that using "puppeteer" is a
> much better suggestion than what I came up with ("integration"). Like
> Matt, I would also be in favour of renaming the suite itself in
> treeherder etc.

:) I agree about renaming in TreeHerder too but didn't want to go on 
that tangent yet in my email. +1



More information about the firefox-dev mailing list