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.
* 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.
* 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.
* 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.
* `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.
* 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.
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?
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.
* 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.
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. 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.
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.
, 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).
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.
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. :-)
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.
~ Gijs
_______________________________________________
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
