Node.js-style --require CLI argument #296

jokeyrhyme · 2015-12-03T05:27:33Z

I figured I'd have an attempt at being able to pass the --require through to the Node executable used for the child process (#229).

My test project uses import and export, so doesn't execute in Node v5 without babel/register. I can manually confirm using npm link that the changes I have so far function as expected.

Besides general comments (very much appreciated), I'm a little bit at a loss as where to start with the tests. Do I need to confirm (with tests) that the ava --require moduleId works? Or is it sufficient to use mocks and confirm that arguments are being passed through as expected?

jamestalmage · 2015-12-03T06:22:27Z

Do I need to confirm (with tests) that the ava --require moduleId works?

That would be my approach, does not need to be fancy.

// test/fixture/install-global.js

global.foo = 'bar'

// test/fixture/validate-installed-global.js

import test from '../../'

test(t => t.is(global.foo, 'bar'));

Then use the API to call validate-... using the require hook you've just implemented.

Overall looks pretty good.

jamestalmage · 2015-12-03T06:33:50Z

api.js

-	return fork(args)
+	var options = {};
+
+	if (Array.isArray(this.require) && this.require.length) {


Just do

if (this.require) { // ... }

You have already guaranteed it is an array of at least length one above.

sindresorhus · 2015-12-04T23:27:08Z

Not sure we ever got an agreement on this:

Would exposing the Node.js binary --require flag while disabling our Babel hook solve your problem

Consider allowing custom test preprocessors #229 (comment)

@jamestalmage ^

jamestalmage · 2015-12-05T01:02:27Z

I don't know if our Babel hook should be disabled automatically. I still have not decided the best way to make it all work.

My current thinking is using something along the lines of #297, but using a better version of istanbul-lib-hook. https://github.com/jamestalmage/transform-chain would be part of it. Combine transform-chain with a way of detecting when hooks are installed, similar to how @novemberborn did it here in nyc.

jokeyrhyme · 2015-12-05T05:13:08Z

Unless I somehow accomplished this accidentally, I did not attempt to disable ava's babel hook. My only purpose with this PR was the pass through the --require.

Also, I hope to get time to address the PR feedback over the next few days. I'm sorry for not being very responsive.

jamestalmage · 2015-12-05T05:27:07Z

Unless I somehow accomplished this accidentally, I did not attempt to disable ava's babel hook

Understood, we are still deciding if / when / how to do that. Once a decision on that gets made we will merge this, and augment it with whatever behavior we finally decide on.

I'm sorry for not being very responsive.

That is not a problem. We still have to answer a few questions, so we are in no rush to merge.

jokeyrhyme · 2015-12-05T07:07:28Z

I still haven't pushed tests yet. They don't seem quite as trivial as I'd hoped, even with @jamestalmage's suggestions. Soon...

jokeyrhyme · 2015-12-08T03:36:52Z

Really stuck with the testing part. I'm getting similar errors when I try to use api.js instead of cli.js.

I've tried a few simple approaches, and none of them work. But this functionality doesn't seem complicated enough to warrant a complicated test. Is there an obvious flaw in this approach? Or do I need to take this up a notch?

jokeyrhyme · 2015-12-08T03:54:44Z

Thanks for the hint, @jamestalmage. At least now, the errors are all the same one:

Test files must be run with the AVA CLI

jokeyrhyme · 2015-12-08T04:02:25Z

I could use mockery and just confirm that Node.js is called with the expected parameters, without actually calling Node.js? /shrug

jamestalmage · 2015-12-08T05:27:35Z

Well, I'm having a hard time getting this to work on travis. Test #4 seems to work on OSX. Note that the --require flag does not work on Node <4

jamestalmage · 2015-12-08T05:28:10Z

@sindresorhus @vdemedes - either of you develop on linux? Any advice on how he should fix this?

jokeyrhyme · 2015-12-08T10:26:56Z

So I should detect the version of Node in the tests and only run them for >=4.0?
And there's still something funny going on with the paths. The fact that it's so hard to test properly makes me wonder if I've taken a good approach in the first place. :S

jamestalmage · 2015-12-08T12:15:25Z

I don't know. Maybe it would just be better to manually implement the same semantics as the --require flag ourselves. You could just do require(somePath), for every --require== flag inside babel.js. Might end up being more reliable.

sindresorhus · 2015-12-08T23:49:09Z

either of you develop on linux? Any advice on how he should fix this?

I have Linux in a VM, but I almost never use it, so pretty noobish with it.

Maybe it would just be better to manually implement the same semantics as the --require flag ourselves.

👍 This is where the --require magic happens: https://github.com/nodejs/node/blob/04b1a2f7564d70dfdab838d2830e04af34b97fe6/lib/module.js#L469-L488 Which is pretty much just require(resolveCwd(cli.flags.require)); (we already depend on resolve-cwd)

jokeyrhyme · 2015-12-08T23:55:48Z

Okay, well I'll have another crack at it over the next few days. I'll try implementing --require internally so that it works regardless of the Node.js version.

sindresorhus · 2015-12-09T00:01:17Z

api.js

+	if (this.require && !Array.isArray(this.require)) {
+		// normalise single `--require moduleId` usage
+		this.require = [this.require];
+	}


Just use arrify on line 69 and drop this whole thing.

sindresorhus · 2015-12-09T00:06:47Z

requires.forEach(function (el) {
  require(resolveCwd(el));
));

sindresorhus · 2015-12-09T00:09:23Z

test/fixture/install-global.js

@@ -0,0 +1 @@
+global.foo = 'bar'


Weird. I love my semi-colons, too. I wonder why ESLint didn't bust my chops about this? :)

#296 (comment)

jokeyrhyme · 2015-12-10T11:06:40Z

Also, I added 4 tests to check different argument orders, but we should probably just pick the one we like and get rid of the rest?

sindresorhus · 2015-12-10T11:23:46Z

lib/babel.js

@@ -16,6 +16,11 @@ if (debug.enabled) {
 // Bind globals first, before anything has a chance to interfere.
 var globals = require('./globals');

+var resolveCwd = require('resolve-cwd');
+(opts.require || []).forEach(function (moduleId) {


I think opts.require is already guaranteed to be an array, so this check is moot.

https://github.com/sindresorhus/ava/pull/296/files#diff-2cce40143051e25f811b56c79d619bf5R73

Nope. In the API tests, new Api() is called without the second parameter, which means it ends up being an empty Object, without a "require" property. Should I change the fallback default value in api.js to include an empty "require" array? e.g. this.options = options || { require: [] }; ?

Oh, never mind then. I read it wrong.

jokeyrhyme · 2015-12-10T23:33:36Z

Okay, so in total I have 1 api test and 1 cli test. I left the cli test in there to stress the end-to-end process a bit. Of course, if you think this is overkill then I'm happy to drop the cli test completely.

sindresorhus · 2015-12-10T23:38:38Z

readme.md

@@ -697,4 +698,3 @@ Concurrency is not parallelism. It enables parallelism. [Learn more.](http://sta
 	<br>
 	<br>
 </div>
-


Don't remove

sindresorhus · 2015-12-10T23:40:09Z

I left the cli test in there to stress the end-to-end process a bit

I think it's overkill to CLI test all options, but I'm ok with this if you rework it into being a generic CLI option test, meaning change the test description. We should have one CLI test making sure options are passed correctly.

jokeyrhyme · 2015-12-11T00:18:49Z

I gave it a whirl, but it seemed infeasible to mock the boundary from cli.js to the others, and my other approach to CLI argument tests seemed unnecessarily complex (and still didn't work).

If we still feel strongly about having CLI tests, we can have a separate Issue / PR.

sindresorhus · 2015-12-11T00:24:38Z

LGTM

vadimdemedes · 2015-12-11T07:34:25Z

LGTM

Node.js-style --require CLI argument

sindresorhus · 2015-12-11T11:01:05Z

Thank you @jokeyrhyme! :) 💥👌

Keep the quality contributions coming. We really appreciate your help.

vadimdemedes · 2015-12-11T11:03:07Z

Thank you for your work, @jokeyrhyme!

ariporad · 2015-12-14T01:11:14Z

Hey, @sindresorhus, @vdemedes, @jamestalmage: Any idea when this might ship? I'd really prefer to not depend directly on master. Thanks!

sindresorhus · 2015-12-14T01:21:26Z

@ariporad We're planning a new release when #326 lands.

ariporad · 2015-12-14T01:22:51Z

Thanks @sindresorhus!

sindresorhus · 2015-12-14T21:40:03Z

@ariporad Actually, I did a release now. https://github.com/sindresorhus/ava/releases/tag/v0.8.0

ariporad · 2015-12-14T23:01:05Z

Yay! Thanks!

jamestalmage · 2016-01-15T06:36:00Z

Any idea why Mocha would be outperforming AVA by such a huge factor, in that case? Are there existing perf issues that you're aware of? (Sorry, not directly related to this PR)

@billyjanitsch

There have been major performance improvements since this was discussed. If possible, could you give AVA a try again and tell us how we're doing?

billyjanitsch · 2016-01-15T17:43:39Z

@jamestalmage

That's great to hear!

Currently this is the biggest migration blocker for us: it's important that our tests run in the same transpilation environment as our lib code (we now use several custom Babel transforms). Our test suite is large enough (>100 files) that I'd rather not rewrite it in AVA until I know this will be possible.

That said, I'd be happy to convert a couple of our tests to see what the perf looks like relative to Mocha.

novemberborn · 2016-01-15T18:02:57Z

@billyjanitsch

Currently this is the biggest migration blocker for us: it's important that our tests run in the same transpilation environment as our lib code (we now use several custom Babel transforms).

My approach to this is to transpile using babel-cli and run the tests on the transpiled code as it sits on the disk. That way I know that I'm testing the code that I'll end up publishing.

See identifierfy for an example.

jamestalmage · 2016-01-15T18:32:05Z

Our test suite is large enough (>100 files) ...

Wow! I would love to see a project that big converted. Is it open source? What is your current test setup? (mocha with chai for assertions, etc)

You may also want to wait for customizable assertions, so we can create one that matches your current assertion api exactly.

billyjanitsch · 2016-01-15T21:58:55Z

@novemberborn

My approach to this is to transpile using babel-cli and run the tests on the transpiled code as it sits on the disk.

I think you're talking about transpiling lib code (imported by tests). This PR provides an easier way to do that: --require 'babel-core/register'.

I'm talking about having test code transpiled in the same way that your lib code is, rather than letting AVA transpile it. See the comment that I linked to for more info.

@jamestalmage

Is it open source? What is your current test setup?

Unfortunately not. I work on Kensho's front-end and we're currently using Mocha/Chai (assert style). I'd like to move to a flat Tape/AVA-style interface.

We do have an open source React chart lib -- also using Mocha but faking a Tape-ish API -- that we might be able to convert as well.

You may also want to wait for customizable assertions, so we can create one that matches your current assertion api exactly.

I'm actually not a huge fan of our current assertions, so I don't mind rewriting them when the time comes. AVA's defaults seem great.

jamestalmage · 2016-01-15T22:04:49Z

@billyjanitsch #424 (comment)

jokeyrhyme changed the title ~~WIP: ava --require babel/register works (needs tests)~~ WIP: ava --require babel/register works Dec 3, 2015

jamestalmage reviewed Dec 3, 2015
View reviewed changes

jokeyrhyme changed the title ~~WIP: ava --require babel/register works~~ WIP: pass --require through to Node.js exec Dec 5, 2015

sindresorhus reviewed Dec 9, 2015
View reviewed changes

sindresorhus mentioned this pull request Dec 9, 2015

Improve passing data to forked processes #318

Closed

sindresorhus reviewed Dec 10, 2015
View reviewed changes

Node.js-style --require CLI argument

6081389

sindresorhus added a commit that referenced this pull request Dec 11, 2015

Merge pull request #296 from jokeyrhyme/pass-through-requires

d5b25f2

Node.js-style --require CLI argument

sindresorhus merged commit d5b25f2 into avajs:master Dec 11, 2015

jokeyrhyme deleted the pass-through-requires branch December 11, 2015 20:05

jokeyrhyme mentioned this pull request Dec 14, 2015

Consider allowing custom test preprocessors #229

Closed

Node.js-style --require CLI argument #296

Node.js-style --require CLI argument #296

Conversation

jokeyrhyme commented Dec 3, 2015

jamestalmage commented Dec 3, 2015

Choose a reason for hiding this comment

sindresorhus commented Dec 4, 2015

jamestalmage commented Dec 5, 2015

jokeyrhyme commented Dec 5, 2015

jamestalmage commented Dec 5, 2015

jokeyrhyme commented Dec 5, 2015

jokeyrhyme commented Dec 8, 2015

jokeyrhyme commented Dec 8, 2015

jokeyrhyme commented Dec 8, 2015

jamestalmage commented Dec 8, 2015

jamestalmage commented Dec 8, 2015

jokeyrhyme commented Dec 8, 2015

jamestalmage commented Dec 8, 2015

sindresorhus commented Dec 8, 2015

jokeyrhyme commented Dec 8, 2015

Choose a reason for hiding this comment

sindresorhus commented Dec 9, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jokeyrhyme commented Dec 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jokeyrhyme commented Dec 10, 2015

Choose a reason for hiding this comment

sindresorhus commented Dec 10, 2015

jokeyrhyme commented Dec 11, 2015

sindresorhus commented Dec 11, 2015

vadimdemedes commented Dec 11, 2015

sindresorhus commented Dec 11, 2015

vadimdemedes commented Dec 11, 2015

ariporad commented Dec 14, 2015

sindresorhus commented Dec 14, 2015

ariporad commented Dec 14, 2015

sindresorhus commented Dec 14, 2015

ariporad commented Dec 14, 2015

jamestalmage commented Jan 15, 2016

billyjanitsch commented Jan 15, 2016

novemberborn commented Jan 15, 2016

jamestalmage commented Jan 15, 2016

billyjanitsch commented Jan 15, 2016

jamestalmage commented Jan 15, 2016