Open Bug 1564431 Opened 5 years ago Updated 2 years ago

[discussion] Unify test folder structure

Categories

(DevTools :: General, task, P3)

task

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 under devtools/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?

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

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.

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #2)

My idea was to split those into mochitest and node 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 think xpcshell would make more sense, but we already use unit today quite consistently for those)
  • node unit tests (jest/mocha) -> node

(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)

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 ?

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?

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?

Let's mention it one last time during the weekly meeting and then start renaming?

Sure!

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, ...

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 ;)

Flags: needinfo?(ogasidlo)
Flags: needinfo?(jlaster)

No opinion, just wanted a good way to surface these bugs in bugzilla so i could follow along offline.

Flags: needinfo?(jlaster)

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? :)

Flags: needinfo?(ogasidlo)

By reading comment 6, 7 and 9, it looks like there is an agreement on xpcshell.

(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.

@Alexandre:
Perfect!

@Julian:
Yes! That was also my initial thought. :)

Thanks everyone for all your feedback and the great discussion! :)

Priority: -- → P3

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.

Just to summarize what we agreed on:

  • browser mochitests -> browser
  • chrome mochitests -> chrome
  • xpcshells -> xpcshell
  • node tests (jest/mocha) -> node
Depends on: 1572008
Depends on: 1589597

I'll try to do this cleanup as I was about to add yet another unique way to add a test folder.

Assignee: nobody → poirot.alex

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.

Depends on: 1609716
Depends on: 1609726

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Component: General → about:debugging
Component: about:debugging → General
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.