CLI: Introduce QUnit CLI #1115

trentmwillis · 2017-03-16T22:04:46Z

As discussed in #1024. This PR will introduce the bare-bones functionality of running tests.

Demo:

Todo:

Get new version of js-reporters for a better TapReporter (which is what tests are assuming)

Krinkle · 2017-03-16T22:41:46Z

bin/qunitjs

+var walkSync = require( "walk-sync" );
+var run = require( "./run" );
+
+var pkg = fs.readJsonSync( path.resolve( __dirname, "..", "package.json" ) );


would require( '../package.json' ); work here? I believe require, unlike fs, is always scoped to where the code is authored, so it shouldn't need __dirname.

Krinkle · 2017-03-16T22:42:47Z

bin/run.js

+
+module.exports = function run( files ) {
+  var QUnitFile = path.resolve( __dirname, "../dist/qunit.js" );
+  var QUnit = require( QUnitFile );


Same here :)

Krinkle · 2017-03-16T23:07:35Z

A few design questions.

Environment

Do we want to make individual test suites executable (even if just optional), or should we require a custom environment with certain predefined globals?
The current draft goes for the latter, which is similar to Mocha's command-line runner. This has the benefits of shorter test files (less bootstrapping), more control over the output, central place for configuration (command line options, or declarative settings file).
The potential downsides are: Requiring linters to know about the environment (ESLint already has a qunit env, though it whitelists way too much; we just need QUnit).
I don't think there are any other downsides, so.. looks good. It also has the benefit of allowing test suites to be shared between traditional browser environments and Node.js, since the former would effectively have QUnit predefined as well.

Source code

How will source code be called from the test suite? If we target Node.js only, then we can just prescribe users to call require() to import any relevant files or modules. On the other hand, this may complicate sharing of test suites with browser environments.

Or would it? That depends on the use case for running tests in Node.js:

Testing in Node.js, in addition to testing in browsers. For libraries that support both environments.
Testing in Node.js //instead// of web browsers. For libraries primarily meant for browsers that don't actually depend on browser APIs, using Node to easily and quickly run the tests without the overhead of web browsers.

In case 1, you'd expect the source code to have built-in support for module.exports and export there instead of globally. But in either case, using require() to load the code should work fine. The main issue is the test files. We'd want to avoid having to put the following in each test file: var MyLib = typeof require !== undefined ? 'require('../') : MyLib;.

On the other hand, if the alternative trying to specify in a declarative way which modules to load in which variables, that may be even less desirable. Here's a quick sketch:

// tests/.qunitrc.js
module.exports = {
  reporter: 'tap',
  predef: {
    MyLib: require( '../' )
  }
};

# tests/.qunitrc.yml
reporter: tap
predef:
  MyLib: '../'

We can learn from ESLint with regards to supporting arbitrary names as reporter and automatically attempting to find an installed npm module with a matching name/prefix.

trentmwillis · 2017-03-16T23:34:26Z

Thanks for the review and input on the design!

Do we want to make individual test suites executable (even if just optional)...

Not entirely sure what you mean by this in relation to the rest of your point.

It also has the benefit of allowing test suites to be shared between traditional browser environments and Node.js, since the former would effectively have QUnit predefined as well.

Yes, this is something I have had specific requests for. It may not be as necessary in the future where ES module import/export is supported in all environments, but I think that is a long way off. So for now, I think having it as an implicit global will be fine, especially since we dropped many globals with the 2.x release.

We can learn from ESLint with regards to supporting arbitrary names as reporter and automatically attempting to find an installed npm module with a matching name/prefix.

Yes, Mocha works this way as well, and I think it makes sense. That said, I would like to also support something like a .qunitrc.js file so that if you want a custom reporter, you can just include it as a constructor function directly. I think the predef idea is also good, as I would like to allow users to run tests in both browser and Node without having to add additional logic or build steps into their code.

Krinkle · 2017-03-16T23:49:43Z

Do we want to make individual test suites executable (even if just optional)...

Not entirely sure what you mean by this in relation to the rest of your point.

I meant the difference between qunit tests/foo.js and node tests/foo.js. The latter is more standalone and is more like how people use assert (as opposed to Mocha), but it would require set up to happen within the file (as part of require( 'qunit' ); and maybe one method call to start the test). This is more flexible for the end-user and makes everything explicit and easy to debug, but it also requires more boilerplate within each test file. It will also probably end up more with a higher learning curve, more back-compat cruft, mistakes, and less flexibility on our side to improve things and take control of output (e.g. instead of relying on the default exception and exit code handling).

With regards to debugging (and other Node.js or V8 command-line flags, like --harmony, which people will want to be able to conveniently set when needed):

# Standalone
node --inspect tests/foo.js

# Custom environment, only works if we explicitly detect and forward it
qunit --node-inspect tests/foo.js

# Manually..
node --inspect ./node_modules/.bin/qunit foo.js

trentmwillis · 2017-03-17T00:45:03Z

I meant the difference between qunit tests/foo.js and node tests/foo.js.

Ah, okay, I see now. There is nothing to preclude users from setting up their own boilerplate. If this is a feature that winds up being needed/wanted, we could likely enable a mode where we don't do any global injections.

As for the debugging aspect, I agree that the extra overhead is a bit annoying, but it's the same for all Node CLIs that I've used. If you ideas/opinions on the best way to support that, I'm all ears!

Krinkle · 2017-03-17T23:54:56Z

As for the debugging aspect, I agree that the extra overhead is a bit annoying, but it's the same for all Node CLIs that I've used. If you ideas/opinions on the best way to support that, I'm all ears!

Yeah. Manual wrapping will work for now. And upstream may provide an environment variable in the future to make this easier to trigger in situations where it's not feasible to pass command-line flags directly to the Node process.

leobalter · 2017-03-18T23:20:14Z

bin/utils.js

+  let globs = args.slice();
+
+  if ( !globs.length ) {
+    globs.push( "test/*.js" );


suggestion, maybe we should include: test/**/*.js, *.jsx and .mjs (node es modules filename, it's ugly, yes)

Seems good to do the recursive thing.

For the other extensions:

Is .mjs officially supported yet?

Can you actually require .jsx files? I was under the impression they needed to be compiled first.

leobalter · 2017-03-18T23:20:39Z

bin/utils.js

+    } catch ( e ) {}
+
+    if ( stat && stat.isDirectory() ) {
+      return `${glob}/*.js`;


leobalter · 2017-03-18T23:21:50Z

bin/utils.js

+    let stat;
+
+    try {
+      stat = fs.statSync( glob );


maybe we can explore async + await features?

leobalter · 2017-03-18T23:22:43Z

bin/utils.js

+
+  if ( !files.length ) {
+    error( "No files were found matching: " + args.join( ", " ) );
+  }


I would not care about it. If files is empty, we should not log errors.

I actually think this is useful feedback for users and ensures the process properly exits with a non-zero code in case they accidentally mess up an input path.

leobalter · 2017-03-18T23:24:40Z

package.json

@@ -58,13 +68,15 @@
    "browserstack": "grunt build && sh build/run-browserstack.sh",
    "build": "grunt build",
    "test": "grunt",
-    "coverage": "grunt coverage"
+    "coverage": "grunt coverage",
+    "test:cli": "grunt build && npm link && qunit test/cli/*.js"


I'm a noob on npm features, this will call qunit set in the bin property in this same file, here?

Yeah, by calling npm link we can call qunit as actual users will after installing it.

@trentmwillis You shouldn't need npm link: npm will prepend ./node_modules/.bin to PATH when this npm script is run.

It failed on a previous run. We can mess around with it after this lands, but for now I don't feel it is worth digging into.

leobalter · 2017-03-18T23:25:34Z

test/cli/main.js

+
+    assert.equal( execution.code, 0 );
+    assert.equal( execution.stderr, "" );
+    assert.equal( execution.stdout, "TAP version 13\nok 1 1\nok 2 1\n1..2" );


wow (the whole test)

leobalter

This seems great for a MVP. I left a few suggestions but they should not block this PR, we can work on follow ups if preferred.

On the walk paths, I remember some work from test262-harness to capture files glob.

trentmwillis · 2017-03-19T18:50:18Z

I've updated this. I think we'll be good to land once a new version of js-reporters is released with qunitjs/js-reporters#95 included.

I'm not super pleased with the tests being coupled to the output format of the TapReporter but couldn't think of a better way to ensure the correct tests are being run, so if anyone has ideas let me know!

platinumazure · 2017-03-20T03:11:44Z

Could I get a quick explanation of the rationale for setting the exit code equal to the number of test failures?

I ask because I've seen another project (ESLint) make a decision about exit codes and then refusing to change that logic when it otherwise seemed worthwhile due to the possibility of breaking CI integration scripts that depend on exit code values too heavily.

I'm questioning whether there is a lot of value in having the exit code be the number of test failures, when that information should be available in stdout (i.e., in the reporter's output).

I'd much rather use exit codes to actually distinguish between a few different failure scenarios. For example:

Exit code 0 means success (obviously)
Exit code 1 means tests failed, but no unexpected errors
Exit code 2 means an unexpected error occurred that prevented the tests from running to completion
- We could use more codes to distinguish between these sub-scenarios: Global timeout (all tests combined took too long), CLI args invalid, test file (when specified as a file or directory rather than glob) couldn't be found, error was thrown with notrycatch config enabled, etc.

So personally I'd rather just use codes for specific types of issues/errors and let consumers read the output for further information. But I could definitely be missing something.

leobalter · 2017-03-20T03:56:00Z

As in ES specs, it would be nice to have some max-min approach. If we land this only with 1 in the exit, we can defer this discussion on the exit values for later and still have the CLI ready.

I'm in between the two approaches. Returning the number of tests or having some other sort of meaningful error codes. Both looks nice but I tend to prefer the one suggested by @platinumazure. We can already consume the failures but it's nice if QUnit can tell it's an unexpected exit.

platinumazure

Should we use tab indentation instead of two-space indentation? This would conform to the jQuery style guide.

trentmwillis · 2017-03-21T05:33:54Z

Could I get a quick explanation of the rationale for setting the exit code equal to the number of test failures?

Just the first thing I did, no super special reason for it. I'm fine with using specific error codes, so I'll update this.

trentmwillis · 2017-03-22T16:30:10Z

Okay, I've updated with the new js-reporters version and addressed all comments (from what I can tell). @platinumazure would appreciate another review from you 😄

leobalter · 2017-03-22T22:52:12Z

I'm amazed to see this work. This is one of the most anticipated features for QUnit.

trentmwillis · 2017-03-24T00:39:25Z

@platinumazure sorry to ping again, but if you can't get to it this evening, I'm planning on merging and can address any potential concerns in follow up PRs. Need this one to land so I can do the other features.

leobalter · 2017-03-24T00:56:27Z

+1 for this.

…

On Thu, Mar 23, 2017 at 5:39 PM Trent Willis ***@***.***> wrote: @platinumazure <https://github.com/platinumazure> sorry to ping again, but if you can't get to it this evening, I'm planning on merging and can address any potential concerns in follow up PRs. Need this one to land so I can do the other features. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1115 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AASYkWq_TcYDl1sHtVVnVIZNad4xMuuyks5roxC-gaJpZM4Mf8Bj> .

platinumazure

Sorry for the delay. LGTM, thanks!

Krinkle reviewed Mar 16, 2017

View reviewed changes

trentmwillis force-pushed the cli branch 2 times, most recently from 8cb509b to 33b8eba Compare March 17, 2017 19:08

trentmwillis force-pushed the cli branch 3 times, most recently from ccb52af to 705fa22 Compare March 18, 2017 17:37

leobalter reviewed Mar 18, 2017

View reviewed changes

bin/utils.js Outdated

} catch ( e ) {}

if ( stat && stat.isDirectory() ) {

return `${glob}/*.js`;

Copy link

Member

leobalter Mar 18, 2017

Choose a reason for hiding this comment

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

same here

leobalter reviewed Mar 18, 2017

View reviewed changes

bin/utils.js Outdated

let stat;

try {

stat = fs.statSync( glob );

Copy link

Member

leobalter Mar 18, 2017

Choose a reason for hiding this comment

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

maybe we can explore async + await features?

leobalter reviewed Mar 18, 2017

View reviewed changes

leobalter approved these changes Mar 18, 2017

View reviewed changes

trentmwillis self-assigned this Mar 19, 2017

platinumazure suggested changes Mar 20, 2017

View reviewed changes

Trent Willis added 2 commits March 22, 2017 09:21

CLI: Introduce QUnit CLI

1847361

Tests: Add CLI tests

e0201fc

trentmwillis force-pushed the cli branch from 84b2451 to e0201fc Compare March 22, 2017 16:22

trentmwillis changed the title ~~[WIP] CLI: Introduce QUnit CLI~~ CLI: Introduce QUnit CLI Mar 22, 2017

platinumazure approved these changes Mar 24, 2017

View reviewed changes

trentmwillis merged commit c65a374 into qunitjs:master Mar 24, 2017

trentmwillis mentioned this pull request Mar 25, 2017

Introduce CLI tool #1024

Closed

trentmwillis deleted the cli branch March 2, 2018 16:58

Krinkle mentioned this pull request Jun 20, 2020

dependencies should use version ranges #1413

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLI: Introduce QUnit CLI #1115

CLI: Introduce QUnit CLI #1115

trentmwillis commented Mar 16, 2017 •

edited

Loading

Krinkle Mar 16, 2017

Krinkle Mar 16, 2017

Krinkle commented Mar 16, 2017

trentmwillis commented Mar 16, 2017 •

edited by Krinkle

Loading

Krinkle commented Mar 16, 2017

trentmwillis commented Mar 17, 2017

Krinkle commented Mar 17, 2017 •

edited

Loading

leobalter Mar 18, 2017

trentmwillis Mar 19, 2017

leobalter Mar 18, 2017

leobalter Mar 18, 2017

leobalter Mar 18, 2017

trentmwillis Mar 19, 2017

leobalter Mar 18, 2017

trentmwillis Mar 19, 2017

platinumazure Mar 20, 2017

trentmwillis Mar 21, 2017

leobalter Mar 18, 2017 •

edited

Loading

leobalter left a comment

trentmwillis commented Mar 19, 2017

platinumazure commented Mar 20, 2017

leobalter commented Mar 20, 2017

platinumazure left a comment

trentmwillis commented Mar 21, 2017

trentmwillis commented Mar 22, 2017

leobalter commented Mar 22, 2017

trentmwillis commented Mar 24, 2017

leobalter commented Mar 24, 2017 via email

platinumazure left a comment

CLI: Introduce QUnit CLI #1115

CLI: Introduce QUnit CLI #1115

Conversation

trentmwillis commented Mar 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Krinkle commented Mar 16, 2017

Environment

Source code

trentmwillis commented Mar 16, 2017 • edited by Krinkle Loading

Krinkle commented Mar 16, 2017

trentmwillis commented Mar 17, 2017

Krinkle commented Mar 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leobalter Mar 18, 2017 • edited Loading

Choose a reason for hiding this comment

leobalter left a comment

Choose a reason for hiding this comment

trentmwillis commented Mar 19, 2017

platinumazure commented Mar 20, 2017

leobalter commented Mar 20, 2017

platinumazure left a comment

Choose a reason for hiding this comment

trentmwillis commented Mar 21, 2017

trentmwillis commented Mar 22, 2017

leobalter commented Mar 22, 2017

trentmwillis commented Mar 24, 2017

leobalter commented Mar 24, 2017 via email

platinumazure left a comment

Choose a reason for hiding this comment

trentmwillis commented Mar 16, 2017 •

edited

Loading

trentmwillis commented Mar 16, 2017 •

edited by Krinkle

Loading

Krinkle commented Mar 17, 2017 •

edited

Loading

leobalter Mar 18, 2017 •

edited

Loading