[discussion] Unify test folder structure
Categories
(DevTools :: General, task, P3)
Tracking
(Not tracked)
People
(Reporter: ogasidlo, Assigned: ochameau)
References
(Blocks 2 open bugs)
Details
As I refactored the folder structure for the application panel recently, we started an interesting conversation about our test folder structure across projects.
:Jdescottes mentioned in one of his comments following:
For aboutdebugging-new, framework and accessibility, the jest test folder is
devtools/client/{name}/test/jest/
(with tests under jest/components/).
Here we dropped the jest folder, and both config files and tests are underdevtools/client/application/test/components/
Could we reuse the same folder structure for concistency?
I was thinking about this for a bit. I agree with consistency but not fully with the general naming convention to add the tool to the folder name as I prefer to focus on the purpose.
What do people think about this?
Reporter | ||
Comment 1•5 years ago
|
||
Here is a list of the folder structure of a few panels:
aboutdebugging-new:
|_ jest
|_ components
|_browser
|_ unit
accessibility
|_ jest
|_ components
|_browser
|_ monchitest
application
|_ components
|_browser
|_ unit
debugger
|_ monchitest
inspector
// test files are without folder structure although prefixed
memory
|_ chrome
|_browser
|_ unit
netmonitor
|_ unit
// most test files are without folder structure although prefixed
performance-new
|_ chrome
storage
// test files are without folder structure although prefixed
webconsole
|_ chrome
|_ components
|_ browser
|_ fixtures
|_ middleware
|_ monchitest
|_ store
|_ unit
|_ utils
Comment 2•5 years ago
|
||
The webconsole structure is quite bad, it forces us to have these tasks in package.json
"test": "mocha \"./{,@(components|middleware|store|utils)/**/}*.test.js\" -r mock-local-storage -r jsdom-global/register -r ./mocha-test-setup.js",
"test-ci": "mocha \"./{,@(components|middleware|store|utils)/**/}*.test.js\" -r mock-local-storage -r jsdom-global/register -r ./mocha-test-setup.js --reporter json"
basically targeting all the "mocha" subfolders, which is terrible.
My idea was to split those into mochitest
and node
so it's more clear.
Comment 3•5 years ago
|
||
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #2)
My idea was to split those into
mochitest
andnode
so it's more clear.
I like node
instead of jest
for all our node-based unit tests.
How many test suites do we care about?
- browser mochitests
- chrome mochitests
- xpc-shell tests
- node unit tests (jest/mocha)
What about
- browser mochitests ->
browser
- chrome mochitests ->
chrome
- xpc-shell tests ->
unit
(I thinkxpcshell
would make more sense, but we already useunit
today quite consistently for those) - node unit tests (jest/mocha) ->
node
Comment 4•5 years ago
|
||
(note that I'm doing some netmonitor launchpad cleanup in https://bugzilla.mozilla.org/show_bug.cgi?id=1567084 and I will move jest tests to a node
subfolder at the same time)
Comment 5•5 years ago
|
||
not a lot of people commented here, so I guess it means everybody agrees? :D
Should we start filing depending bugs and turn this into a meta ?
Reporter | ||
Comment 6•5 years ago
|
||
What about
- browser mochitests ->
browser
- chrome mochitests ->
chrome
- node unit tests (jest/mocha) ->
node
+1 on those suggestions
- xpc-shell tests ->
unit
(I think xpcshell would make more sense, but we already use unit today quite consistently for those)
Not sure about this one as unit
might get confusing, especially for new contributors. Personally I'd prefer xpcshell
as it would as well follow the suggested pattern. Julian, do you have an idea of how much work it would be to refactor the folder nanimg for xpc-shell tests?
Comment 7•5 years ago
|
||
how much work it would be to refactor the folder nanimg for xpc-shell tests?
Thanks for calling that out. It's just renaming, it should not be too bad :) Let's aim for xpcshell
!
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #5)
not a lot of people commented here, so I guess it means everybody agrees? :D
Should we start filing depending bugs and turn this into a meta ?
Let's mention it one last time during the weekly meeting and then start renaming?
Comment 8•5 years ago
|
||
Let's mention it one last time during the weekly meeting and then start renaming?
Sure!
Assignee | ||
Comment 9•5 years ago
|
||
Sorry for not joining this thread sooner, but the suggestion made in comment 6 sounds good.
It looks like the choice between unit
versus xpcshell
isn't clear in mozilla-central.
Within /devtools
we have 30 xcpshell folders:
- 26
unit
folders - 3
test
folders
Outside of /devtools
in the rest of mozilla-central, we have 157 xpcshell folders:
- 81
unit
folders - 34
xpcshell
folders - 12
unit_something
folders - 8
tests
folders - 6
test
folders
So, as of today, unit
wins, but if we do rename all devtools folder, we would shuffle everything and xpcshell
would become the most common pattern. I do agree that it makes sense to use xpcshell
as xpcshell
word will be used everywhere else: mach xpcshell-test
, treeherder UI, mach try
, ...
Assignee | ||
Comment 10•5 years ago
|
||
We discussed this during today's DevTools meeting.
Ola, We weren't sure you were 100% fine with the choice of node
as folder name of all node tests?
Jason, You told us you had some feedback to provide about this change?
If no futher arguments are provided, we will move forward with comment 6 proposal. But we will wait for Ola to get back from PTO, next week ;)
Comment 11•5 years ago
|
||
No opinion, just wanted a good way to surface these bugs in bugzilla so i could follow along offline.
Reporter | ||
Comment 12•5 years ago
|
||
Ola, We weren't sure you were 100% fine with the choice of
node
as folder name of all node tests?
+1 on node
.
Alexandre, just to clarify as this seems like a big change... are we moving on with unit
or xpcshell
? :)
Assignee | ||
Comment 13•5 years ago
|
||
By reading comment 6, 7 and 9, it looks like there is an agreement on xpcshell
.
Comment 14•5 years ago
|
||
(In reply to Ola Gasidlo [:ogasidlo] [:ola PTO until 5th of August] from comment #12)
Ola, We weren't sure you were 100% fine with the choice of
node
as folder name of all node tests?+1 on
node
.
Just wanted to mention that under the test/node
folder, we can still organize test files in subfolders, such as test/node/components
, test/node/actions
. package.json & other config files would still be under test/node
, if that sounds ok.
Reporter | ||
Comment 15•5 years ago
|
||
@Alexandre:
Perfect!
@Julian:
Yes! That was also my initial thought. :)
Thanks everyone for all your feedback and the great discussion! :)
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 16•5 years ago
|
||
Just to be clear, at this stage of the discussion, there is a clear agreement, so anyone should feel free to proceed with this refactoring.
Comment 17•5 years ago
|
||
Just to summarize what we agreed on:
- browser mochitests ->
browser
- chrome mochitests ->
chrome
- xpcshells ->
xpcshell
- node tests (jest/mocha) ->
node
Assignee | ||
Comment 18•5 years ago
|
||
I'll try to do this cleanup as I was about to add yet another unique way to add a test folder.
Assignee | ||
Comment 19•5 years ago
|
||
Here is a first attempt to rename all the xpcshell tests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf1762dd88217477504cbd8467ba6dc0169ff779
Let's see what try says.
Comment 20•5 years ago
|
||
Bugbug thinks this bug should belong to this component, but please revert this change in case of error.
Updated•5 years ago
|
Updated•2 years ago
|
Description
•