1247319 - Create a test wrapper around mocha so it can run in automation

Status: NEW

(DevTools :: Shared Components, defect)

Description

[Brian Grinstead [:bgrins]]

We've been discussing integrating mocha tests into the shared components project.  If we do that, we should make sure they run in automation.  This could be done by creating a mochitest wrapper around a mocha suite.

Comment 1

[Brian Grinstead [:bgrins]]

Attachment: mocha-mochitest-WIP.patch

WIP.  Can be ran with `./mach mochitest test_mocha.html`

Comment 2

[Lin Clark [:linclark]]

I tested this out. It's awesome, but unfortunately I ran into a couple stumbling blocks.

- When trying to execute with mochitest: As soon as you use require, you get a "require is not defined" error
- When trying to execute with mocha: It chokes on object destructuring. It seems this isn't supported in Node yet (https://github.com/nodejs/node/issues/2823). We could potentially use babel just for running mocha tests.

Comment 3

[Lin Clark [:linclark]]

Attachment: demo-mocha-component.patch

This includes a component that's totally presentational which can be used for testing this out.

Comment 4

[Brian Grinstead [:bgrins]]

Attachment: mocha-mochitest-WIP.patch

Updating the test case to match Attachment 8734433 [details] [diff] so loading up the components using the browser loader in the test runner script.  Also, I put a conditional require in the test file so it *might* work from the command line.  I haven't been able to test that though - Lin, can you confirm?

Comment 5

[Lin Clark [:linclark]]

Attachment: Bug1247319-mocha-mochitest-WIP.patch

I got it working with a couple of modifications to mocha_pane. I *think* this patch includes everything. 

It also includes the package.json file and .babelrc you need to run it via command line. To test that:

> cd devtools
> npm i
> npm test

That should work, ping me if it doesn't.

Comment 6

[Lin Clark [:linclark]]

I should note (before someone asks): The package.json and .babelrc are just there for testing purposes. As discussed in the React components meeting, we don't have to commit them.

Comment 7

[Lin Clark [:linclark]]

Attachment: Bug1247319-mocha-mochitest-WIP.patch

This patch tests one of the Reps. It works out of the box for the mochitest. For the command line, it requires two changes to the rep's component file:

1. Use relative paths for module loading (as we talked about in the meeting)
2. If `define` is not a function (e.g. in Node), use the amdefine module (provided by requirejs) to define it

James mentioned something about doing the paths in a babel transform. I've never written a babel transform, so I don't know what this would look like. James, could you take a look and see whether you think a babel transform makes sense in this case?

Comment 8

[James Long (:jlongster)]

For context: I'm hesitant to force everything to be relative. It would be hard for components in the tools themselves, like `devtools/client/debugger/content/components`. To get React I'd have to do many `..`, at least if I'm understanding this correctly. We'll use this testing framework for app-specific components too.

We definitely will need to figure out how to map the paths. I really wish node had an extensible module loader. Would it work to munge the NODE_PATH variable and add the `gecko-dev` path to it, so that `devtools/client/...` would actually resolve?

If not, yeah we could probably write a babel loader. I've never actually done it either but it doesn't look too hard. I've dealt with JS ASTs before, and you're looking for top-level require statements and statically making the `devtools/...` paths relative. The NODE_PATH idea is interesting though, would that work?

Comment 9

[Lin Clark [:linclark]]

So Node *kinda* has an extensible module loader. You can monkey patch require, and I had thought about suggesting this as a potential option. Here's some info: https://glebbahmutov.com/blog/hacking-node-require/

I think our use of Node would be limited enough here that we wouldn't really encounter any downsides that are associated with monkey patching.

Comment 10

[James Long (:jlongster)]

(In reply to Lin Clark [:linclark] from comment #9)
> So Node *kinda* has an extensible module loader. You can monkey patch
> require, and I had thought about suggesting this as a potential option.
> Here's some info: https://glebbahmutov.com/blog/hacking-node-require/
> 
> I think our use of Node would be limited enough here that we wouldn't really
> encounter any downsides that are associated with monkey patching.

I would be fine with it generally, although I'm not familiar what the potential consequences are. We'd have to try it for a while to learn.

Does putting `/Users/james/projects/geck-doev` in NODE_PATH not fix it? We only need that one path to be global.

Comment 11

[James Long (:jlongster)]

(not that specific path of course, whatever the path is to the sourcedir)

Comment 12

[Lin Clark [:linclark]]

Good point. And we can actually put that in the npm test command so that it doesn't have to be set by the user. This will make it work better for anyone who uses a git new-workdir workflow.

So the npm script becomes:

> "test": "NODE_PATH=`pwd`/../ mocha client/shared/components/test/mochitest/mocha_pane.js --compilers js:babel-register"

Thanks for the suggestion!

Comment 13

[James Long (:jlongster)]

Sweet! Yeah I was hoping it would happen behind the scenes. Thanks so much for working on this, pretty exciting!

Comment 14

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

Comment on attachment 8734517 [details] [diff] [review]
Bug1247319-mocha-mochitest-WIP.patch

Review of attachment 8734517 [details] [diff] [review]:
-----------------------------------------------------------------

Seems reasonable overall, thanks for working on it!  The NODE_PATH approach sounds nice to me.

::: devtools/client/shared/components/reps/object-box.js
@@ +6,5 @@
>  
>  "use strict";
>  
> +if (typeof define !== 'function') {
> +  var define = require('amdefine')(module);

Since we need to use "amdefine" for Node here anyway, we could use that has a way to be more explicit in our own loader: instead of magically adding "define" as a global in our loaders[1][2], we could offer "amdefine" as a pre-defined module, like others we have today[3].

This way we can support both Node and in-product AMD use cases without having to do the conditional `if (typeof define !== 'function') {` check.  Requiring a module to get define, even if it's a "special" module, seems slightly less surprising than "define" being magically available.

[1]: https://dxr.mozilla.org/mozilla-central/rev/d5f3da0cfe7ccf846c354014c9b059fad6ba0de5/devtools/shared/Loader.jsm#308
[2]: https://dxr.mozilla.org/mozilla-central/rev/d5f3da0cfe7ccf846c354014c9b059fad6ba0de5/devtools/client/shared/browser-loader.js#124
[3]: https://dxr.mozilla.org/mozilla-central/rev/d5f3da0cfe7ccf846c354014c9b059fad6ba0de5/devtools/shared/Loader.jsm#31