Open Bug 1247319 Opened 4 years ago Updated Last year

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

Categories

(DevTools :: Shared Components, defect)

defect
Not set

Tracking

(Not tracked)

People

(Reporter: bgrins, Unassigned)

Details

Attachments

(1 file, 4 obsolete files)

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.
Attached patch mocha-mochitest-WIP.patch (obsolete) — Splinter Review
WIP.  Can be ran with `./mach mochitest test_mocha.html`
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.
Attached patch demo-mocha-component.patch (obsolete) — Splinter Review
This includes a component that's totally presentational which can be used for testing this out.
Attached patch mocha-mochitest-WIP.patch (obsolete) — Splinter Review
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?
Attachment #8717961 - Attachment is obsolete: true
Flags: needinfo?(lclark)
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.
Attachment #8734433 - Attachment is obsolete: true
Attachment #8734463 - Attachment is obsolete: true
Flags: needinfo?(lclark)
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.
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?
Attachment #8734476 - Attachment is obsolete: true
Flags: needinfo?(jlong)
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?
Flags: needinfo?(jlong)
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.
(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.
(not that specific path of course, whatever the path is to the sourcedir)
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!
Sweet! Yeah I was hoping it would happen behind the scenes. Thanks so much for working on this, pretty exciting!
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
Attachment #8734517 - Flags: feedback+
No longer blocks: 1257552
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.