Add flag to require modules before running tests #224

mstade · 2015-12-19T20:45:17Z

By adding the --require flag (and -r alias) we allow users to require modules before running their tests. An example of this might be adding source-map-support, like so:

$ tape -r source-map-support/register test/*.js

Another example might be to install the babel require hook:

$ tape -r babel-register test/*.js

Another potential benefit is the ability to more easily run external test suites:

$ npm install test-suite-rfc1337
$ tape -r test-suite-rfc1337

Modules are found using the node module resolution algorithm, so if one wants to load a local module it is necessary to prepend ./ or ../, like so:

$ tape -r ./my/local/module

The `source-map-support` module enables some rudimentary support for source maps in call stacks, which greatly enhances the developer experience of working with compiled code.

Raynos · 2015-12-20T01:08:48Z

I don't understand what did does.

Tape doesn't do anything with source maps. Why add it here ?

mstade · 2015-12-20T01:51:14Z

My apologies, the description wasn't particularly clear. If the code being run is linked to source maps, then any errors thrown will have a call stack that reflects the original code. Without it, it's much more difficult to understand where errors originate. Does that help?

Raynos · 2015-12-20T11:48:19Z

Can you show me what a workflow looks like ?

Would it be something like

make build # compile my XJS -> real js
node build/test.js
# :( get bad stacktrace

I think it might make more sense for some-one to write source-map-node binary that runs any node program but pre-loads source maps support.

This way it's opt-in for people who want source map support and we don't have source map hooks enabled globally for all users of tape.

mstade · 2015-12-20T14:30:06Z

That's pretty much the workflow, yeah.

I think it might make more sense for some-one to write source-map-node binary that runs any node program but pre-loads source maps support.

Hmm, yes, maybe. The problem with more binaries of course is that they pile up. I want to run with source map support, but I also want to run a code coverage collector. But I think you're on to something though. Modules like source-map-support that have a kind of cross cutting module like register can just be loaded like tests would be. In the example above, it becomes:

make build # compile my XJS -> real js
tape node_modules/source-map-support/register build/test.js
# :) get good stacktrace

Except, for whatever reason I can't seem to figure out right now, the above doesn't work. Would it be meaningful to add a -r,-require flag to tape, that would then enable this kind of workflow, yet still be opt-in? Something like:

tape -r source-map-support/register build/test.js

I did a quick hack to test this, and it solves my issue of course. Speculating further, this kind of extension point could potentially enable things like running multiple pre-requisites:

tape -r source-map-support/register -r collect-coverage build/test.js

If one wants to parameterize these modules, or do something more esoteric, you can implement it in a local module:

tape -r ./my-esoteric-prerequisites build/test.js

Happy to change this PR, or make a new one, if you think this is within the purview of tape. Like you, I don't much like the added responsibility this PR puts on tape, which I think is brilliant in its limited scope and simplicity. Adding a -r,--require flag would make tape a little more complex, but I think the added value is potentially much greater than the cost of such a feature. What do you think?

Raynos · 2015-12-20T21:57:24Z

I'm +1 for a -r flag.

Although demonstrating that this is powerful when it comes to for example, combining istanbul & tape together will help get it landed.

If your going to add an options parser to implement -r then please use minimist.

mstade · 2015-12-20T22:18:10Z

Right on, thanks for the discussion @Raynos – much obliged! I'll carve out some time to implement -r in the next few days, ideally before Christmas. I'll just repurpose this PR for it, but if you feel strongly otherwise feel free to close and I'll just open a new one.

This reverts commit 737aa42.

@Raynos

@Raynos accurately pointed out that the source map support was a bit too specific, and felt outside of the purview of tape. As a more general solution, this implements the `-r` and `--require` CLI flags, which will allow a user to load node modules before any tests are run. For example, if someon wants to enable source map support in node, they might do: tape -r source-map-support/register test/*.js

mstade · 2015-12-21T20:59:13Z

@Raynos ok I've removed the source map stuff and implemented -r and --require instead. I also added a couple of tests for it, but I'm not sure of their quality. I've also not changed the documentation any, which is probably something that should also be done.

Any thoughts?

mstade · 2015-12-21T21:22:33Z

I updated the title and description of this PR to reflect the new changes. It makes the conversation look a bit weird, but whatever. :o)

mstade · 2015-12-21T21:48:32Z

Some thoughts:

Order is not preserved

Consider the following:

$ tape -r first second -r third

Order is preserved for flags and globs separately, so the order of execution in the above example is first, third, second. Would it be more intuitive and meaningful to preserve the order entirely? Could this help with things like #126 where the node process never ends because of lingering handles? I.e. if order was preserved, then setup and teardown could be extracted into modules of their own, and common logic shared:

$ tape -r setup-db test/*.test.js -r teardown-db

Sure enough, this can be done without -r as well, by having setup and teardown files:

$ tape test/setup-db.js test/*.test.js test/teardown-db.js

But this makes it more difficult to share such modules, for instance on a team that does a lot of similar work. I'm not sure which way to lean on this.

Parameterizing modules

Parameterizing modules can be done using environment variables:

$ BABEL_ENV=production tape -r babel-register test/*.js

Is there a need for parameterization of modules, that can't be solved with environment variables? If so, should the recommended approach be to implement a local module and load that? E.g.:

$ cat ./my-esoteric-needs
var mod = require('some-module')
mod.installGlobablly({ some: 'parameters' })

$ tape -r ./my-esoteric-needs test/*.js

Raynos · 2015-12-21T21:52:13Z

Order is not preserved

$ tape -r first second -r third

I expect first, third, second. Like first its going to run all the pre requires and then run all the glob patterns.

Working as expected & intended. If you want to feel warm and fuzzy then run tape -r first -r third second

Raynos · 2015-12-21T21:52:43Z

Parameterizing modules

You are spot on; use environment variables or write a local helper.

mstade · 2015-12-21T21:56:47Z

Cool, thanks for the comments @Raynos. I think I agree with you re: the order of execution; it makes sense to run all requires first, regardless of what order they are specified in on the command line.

I'll see about updating the docs as well before I consider my work here done.

Raynos · 2015-12-21T21:56:51Z

package.json

@@ -15,7 +15,9 @@
    "glob": "~5.0.3",
    "has": "~1.0.1",
    "inherits": "~2.0.1",
+    "minimist": "1.2.0",


Please use ~

Raynos · 2015-12-21T21:58:45Z

This looks good to me.

We need to add a section to the README about this new feature, i can do that after merging if you prefer.

I think we should wait 24 hours before merging to let other reviewers like @substack take a look.

This can probably be done better, but I think it covers all aspects of this feature.

mstade · 2015-12-21T22:23:32Z

Thanks for the review @Raynos, I've incorporated your feedback and added some docs to the readme as well. Feel free to review and suggest any changes, but I think that about covers it – what's your take?

ljharb · 2015-12-21T22:31:57Z

bin/tape

+    /* The `module &&` ensures we ignore `-r ""`, trailing `-r` or other
+     * silly things the user might (inadvertedly) be doing.
+     */
+    module && require(resolveModule(module, { basedir: cwd }));


i'd prefer if (module) { require(resolveModule(module, { basedir: cwd })); }, rather than abusing boolean operators for control flow :-/

Potato/tomato in my book, but happy to change it. Will push an update in a minute.

ljharb · 2015-12-21T22:37:13Z

bin/tape

 var glob = require('glob');

-process.argv.slice(2).forEach(function (arg) {
+var opts = parseOpts(process.argv.slice(2), {
+        alias: { 'r': 'require' },


any reason these "r"s are quoted?

Force of habit, really. I'll change it.

ljharb · 2015-12-21T22:40:15Z

Overall this LGTM! It'd be great to rebase this down to a smaller number of meaningful atomic commits

mstade · 2015-12-21T22:42:40Z

Sure, if you decide to merge feel free to rebase any which way you'd like! :o)

mstade · 2015-12-21T22:43:49Z

Thanks for the review btw @ljharb!

ljharb · 2015-12-21T22:45:58Z

Ideally we'd merge through the web interface which means you'd need to do any rebasing.

mstade · 2015-12-21T22:59:17Z

With all due respect – I'm cool with contributing a feature and making sure the code is up to your standards so the patch can be landed, but the bike shedding is strong with this one. Regardless, I'll take @Raynos's cue and await any further feedback from @substack (or others.)

ljharb · 2015-12-21T23:03:31Z

Your contribution is appreciated, but what you call "bikeshedding" involves consistency with a project's style and commit history, and is at the least as important as new features. Certainly if you're unwilling to rebase the commits, and we'd still like the feature, we can either click merge and deal with the log clutter, or manually squash the commits ourselves, but to dismiss maintainer concerns as "bikeshedding" doesn't seem very helpful :-/

Awaiting further feedback seems best in any case.

mstade · 2015-12-21T23:19:45Z

You're right @ljharb – my apologies. I didn't mean to offend anyone, nor dismiss feedback. I very much appreciate the solid feedback provided.

Raynos · 2015-12-22T01:00:56Z

This looks solid. I think the README is fine.

As mentioned I'll wait ~24 hours before merging but if there's no strong objections I'll merge tomorrow.

ljharb · 2015-12-22T03:08:26Z

👍 i'll definitely be using this feature

mstade · 2015-12-22T15:38:31Z

I've rebased and pushed a new version of this branch. I didn't want to mess with this branch, for reasons. Let me know how you'd like to proceed if you'd like to merge this. It's equally trivial to make the same changes to this branch, as it is to open up a new PR just for merging – your call.

Thanks for the reviews and feedback!

Add flag to require modules before running tests

Raynos · 2015-12-23T00:20:18Z

Published 4.3.0

mstade · 2015-12-23T08:36:53Z

Much obliged! 👍

Added rudimentary source map support

737aa42

The `source-map-support` module enables some rudimentary support for source maps in call stacks, which greatly enhances the developer experience of working with compiled code.

mstade added 2 commits December 21, 2015 19:56

Revert "Added rudimentary source map support"

3f02033

This reverts commit 737aa42.

mstade changed the title ~~Added rudimentary source map support~~ Add flag to require modules before running tests Dec 21, 2015

Raynos reviewed Dec 21, 2015
View reviewed changes

package.json

@@ -15,7 +15,9 @@

"glob": "~5.0.3",

"has": "~1.0.1",

"inherits": "~2.0.1",

"minimist": "1.2.0",

Copy link

Collaborator

Raynos Dec 21, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use ~

mstade added 3 commits December 21, 2015 22:59

Use ~ for minimist and resolve dependency versions.

4f81dbc

Make single require check more readable

d0ca885

Add -r,--require documentation to README.md

096d2e7

This can probably be done better, but I think it covers all aspects of this feature.

ljharb reviewed Dec 21, 2015
View reviewed changes

mstade added 3 commits December 21, 2015 23:33

Fix indent mistake

2e57f22

Use regular ol' if instead of boolean operator in sanity check

ffa503a

Fix spelling mistake in comment

4077efe

ljharb reviewed Dec 21, 2015
View reviewed changes

Unquote keys in object literals

9f02249

Raynos added a commit that referenced this pull request Dec 23, 2015

Merge pull request #224 from mstade/source-map-support

62b352b

Add flag to require modules before running tests

Raynos merged commit 62b352b into tape-testing:master Dec 23, 2015

mstade deleted the source-map-support branch December 23, 2015 08:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add flag to require modules before running tests #224

Add flag to require modules before running tests #224

mstade commented Dec 19, 2015

Raynos commented Dec 20, 2015

mstade commented Dec 20, 2015

Raynos commented Dec 20, 2015

mstade commented Dec 20, 2015

Raynos commented Dec 20, 2015

mstade commented Dec 20, 2015

mstade commented Dec 21, 2015

mstade commented Dec 21, 2015

mstade commented Dec 21, 2015

Raynos commented Dec 21, 2015

Raynos commented Dec 21, 2015

mstade commented Dec 21, 2015

Raynos Dec 21, 2015

Raynos commented Dec 21, 2015

mstade commented Dec 21, 2015

ljharb Dec 21, 2015

mstade Dec 21, 2015

ljharb Dec 21, 2015

mstade Dec 21, 2015

ljharb commented Dec 21, 2015

mstade commented Dec 21, 2015

mstade commented Dec 21, 2015

ljharb commented Dec 21, 2015

mstade commented Dec 21, 2015

ljharb commented Dec 21, 2015

mstade commented Dec 21, 2015

Raynos commented Dec 22, 2015

ljharb commented Dec 22, 2015

mstade commented Dec 22, 2015

Raynos commented Dec 23, 2015

mstade commented Dec 23, 2015

Add flag to require modules before running tests #224

Add flag to require modules before running tests #224

Conversation

mstade commented Dec 19, 2015

Raynos commented Dec 20, 2015

mstade commented Dec 20, 2015

Raynos commented Dec 20, 2015

mstade commented Dec 20, 2015

Raynos commented Dec 20, 2015

mstade commented Dec 20, 2015

mstade commented Dec 21, 2015

mstade commented Dec 21, 2015

mstade commented Dec 21, 2015

Order is not preserved

Parameterizing modules

Raynos commented Dec 21, 2015

Raynos commented Dec 21, 2015

mstade commented Dec 21, 2015

Raynos Dec 21, 2015

Choose a reason for hiding this comment

Raynos commented Dec 21, 2015

mstade commented Dec 21, 2015

ljharb Dec 21, 2015

Choose a reason for hiding this comment

mstade Dec 21, 2015

Choose a reason for hiding this comment

ljharb Dec 21, 2015

Choose a reason for hiding this comment

mstade Dec 21, 2015

Choose a reason for hiding this comment

ljharb commented Dec 21, 2015

mstade commented Dec 21, 2015

mstade commented Dec 21, 2015

ljharb commented Dec 21, 2015

mstade commented Dec 21, 2015

ljharb commented Dec 21, 2015

mstade commented Dec 21, 2015

Raynos commented Dec 22, 2015

ljharb commented Dec 22, 2015

mstade commented Dec 22, 2015

Raynos commented Dec 23, 2015

mstade commented Dec 23, 2015