Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Global "only" (#524) #527

Merged
merged 17 commits into from Feb 24, 2016

Conversation

@rluba
Copy link
Contributor

rluba commented Feb 24, 2016

I’ve implemented the global only flag as discussed in #524. Some of the test input files were copied & adapted from @Marsup’s original PR #116.

There are several topics that I’d like to hear everyone’s opinion on:

  1. Currently, the runner simply ignores everything but the first only it encounters. I’d like to report an error instead but I don’t know how. There’s no mechanism for the runner to report general errors (only test errors). Every callback error is simply ignored (see here or here) and not forwarded to the reporters, so they can’t handle it. That’s why two new tests still fail.
  2. The ids / glob feature currently uses its own mechanism for ignoring tests. I’d like to merge this with the skip behavior so that tests ignored by the ids / glob parameters are also simply marked as "skipped" instead of not being part of the reports at all.
  3. The console reporter now prints the number of skipped tests. Shall we add this to other reporters as well?
  4. I removed some 500 lines of boilerplate code from the cli tests by extracting a single function. I’ve also improved some of the assertions to be more precise. Any objections?
  5. The before and after blocks are now consistently skipped for skipped tests. Previously, they were still executed for tests that were skipped individually (eg. using test.skip(…)).
@@ -124,6 +124,11 @@ exports.execute = function (scripts, options, reporter, callback) {
return script._root;
});

const onlyNodes = scripts.reduce((nodes, script) => nodes.concat(script._onlyNodes), []);

This comment has been minimized.

Copy link
@geek

geek Feb 24, 2016

Member

Please wrap in {

const onlyNodes = scripts.reduce((nodes, script) => {

    return nodes.concat(script._onlyNodes);
}, []);

This comment has been minimized.

Copy link
@Marsup

Marsup Feb 24, 2016

Member

Isn't it more readable with a Hoek.flatten(scripts.map((script) => script._onlyNodes)) ? Personal tastes...

This comment has been minimized.

Copy link
@rluba

rluba Feb 24, 2016

Author Contributor

I’ll use @Marsup’s version.

@@ -124,6 +124,11 @@ exports.execute = function (scripts, options, reporter, callback) {
return script._root;
});

const onlyNodes = scripts.reduce((nodes, script) => nodes.concat(script._onlyNodes), []);
if (onlyNodes.length) {
internals.skipEverythingExceptOnlyPath(scripts, onlyNodes[0]);

This comment has been minimized.

Copy link
@geek

geek Feb 24, 2016

Member

consider rename, maybe skipAllButOnly ... needs to be shorter and simpler: skipNononly filterSkip skipExperiments

This comment has been minimized.

Copy link
@rluba

rluba Feb 24, 2016

Author Contributor

skipAllButOnly it is!


return new Promise((resolve, reject) => {

const cli = ChildProcess.spawn('node', [labPath, ...args]);

This comment has been minimized.

Copy link
@geek

geek Feb 24, 2016

Member

no rest arguments supported yet

This comment has been minimized.

Copy link
@rluba

rluba Feb 24, 2016

Author Contributor

It’s the spread operator, but you are right: I just saw that it’s only supported in node 5, not in 4.

This comment has been minimized.

Copy link
@geek

geek Feb 24, 2016

Member

Good to know, thanks.

This comment has been minimized.

Copy link
@Marsup

Marsup Feb 24, 2016

Member

Still behind a flag.

This comment has been minimized.

Copy link
@rluba

rluba Feb 24, 2016

Author Contributor

@Marsup It works in node 5 without a flag (tested on v5.5.0) as it’s not the "rest operator" but the "spread operator". (I find it pretty confusing that they look the same, have similar semantics but have completely different platform support.)


it('returns true when 1 + 1 equals 2', (done) => {

expect(1+1).to.equal(2);

This comment has been minimized.

Copy link
@geek

geek Feb 24, 2016

Member

please space infix operators expect(1 + 1).to.equal(2);

This comment has been minimized.

Copy link
@rluba

rluba Feb 24, 2016

Author Contributor

Sure, I just copied that from #116.

test/cli.js Outdated
@@ -9,7 +9,7 @@ const Code = require('code');
const Lab = require('../');
const Pkg = require('../package.json');
const _Lab = require('../test_runner');

const runCli = require('./run_cli');

This comment has been minimized.

Copy link
@geek

geek Feb 24, 2016

Member

Please uppercase modules... this helps to make it easier to identify what is local and what is external


module.exports = (args, callback) => {

return new Promise((resolve, reject) => {

This comment has been minimized.

Copy link
@geek

geek Feb 24, 2016

Member

Please use the callback style instead of a Promise here

@geek

This comment has been minimized.

Copy link
Member

geek commented Feb 24, 2016

@rluba really well done!

  1. Is the error when no only experiment or test is found?
  2. I like the idea, but would the output show the skipped tests in the console reporter? I am fine with making this change if we only show the skipped totals and not each skipped test in the reporter. This is a feature that we use a lot in hapi and I don't want to make the reporter output too noisy when you only care about running a couple of tests in development.
  3. That would be great to add to the other reporters, but we can definitely do that as a separate PR to make it easier for this one to land.
  4. Looks great... only change is to remove the rest params and to not use a Promise
  5. Awesome!
@@ -147,7 +152,8 @@ exports.execute = function (scripts, options, reporter, callback) {
},
reporter: reporter,
filters: filters,
options: settings
options: settings,
only: onlyNodes[0]

This comment has been minimized.

Copy link
@Marsup

Marsup Feb 24, 2016

Member

I think that'll require a comment for further maintenance, it's not obvious why you picked the 1st.

This comment has been minimized.

Copy link
@rluba

rluba Feb 24, 2016

Author Contributor

@Marsup I hope the new version clarifies that. If not, let me know!

cli.once('close', (code, signal) => {

if (signal) {
reject(new Promise(() => {

This comment has been minimized.

Copy link
@Marsup

Marsup Feb 24, 2016

Member

Shouldn't ESLint pick this padding ?

This comment has been minimized.

Copy link
@rluba

rluba Feb 24, 2016

Author Contributor

@Marsup You mean "no empty line"? Nope, it doesn’t. Only complains for functions, not for conditional blocks.

expect(output).to.contain('10 tests complete');
expect(result.errorOutput).to.equal('');
expect(result.code).to.equal(0);
expect(result.output).to.contain('9 tests complete');

This comment has been minimized.

Copy link
@Marsup

Marsup Feb 24, 2016

Member

Is that normal we lost one ?

This comment has been minimized.

Copy link
@rluba

rluba Feb 24, 2016

Author Contributor

@Marsup Sharp 👀! I had to delete the old "local only" test file because it would have broken the "whole directory" test runs once only worked globally.

test/cli.js Outdated
done();
});
expect(result.errorOutput).to.equal('');
expect(result.output).to.contain('Should execute before 1');

This comment has been minimized.

Copy link
@Marsup

Marsup Feb 24, 2016

Member

Contain supports arrays, you should do that instead. (and on the others below)

This comment has been minimized.

Copy link
@rluba

rluba Feb 24, 2016

Author Contributor

Didn't know that, thx. Normally I use a completely different assertion library because it provides far superior error messages.

const ChildProcess = require('child_process');
const Path = require('path');

const labPath = Path.join(__dirname, '..', 'bin', '_lab');

This comment has been minimized.

Copy link
@Marsup

Marsup Feb 24, 2016

Member

Eran's gonna want his internals :)

This comment has been minimized.

Copy link
@rluba

rluba Feb 24, 2016

Author Contributor

That’s verbatim from the original code but I can change it.

return new Promise((resolve, reject) => {

const cli = ChildProcess.spawn('node', [labPath, ...args]);
let output = '';

This comment has been minimized.

Copy link
@Marsup

Marsup Feb 24, 2016

Member

FWIW it's faster to push in arrays and join at the end, not sure it'll dramatically change test suite time though ;)

@Marsup

This comment has been minimized.

Copy link
Member

Marsup commented Feb 24, 2016

Done reviewing as well, great job.

@rluba

This comment has been minimized.

Copy link
Contributor Author

rluba commented Feb 24, 2016

@geek

Is the error when no only experiment or test is found?

No, no – that would break pretty much every current test case :-) I meant that I’d like to throw an error if there’s more than one only (doesn’t matter if it’s on experiments or tests).

I had this code in place, but there’s simply no place that would handle or output the error:

    // (existing line)
    const onlyNodes = scripts.reduce((nodes, script) => nodes.concat(script._onlyNodes), []);
    // What I’d like to add:
    if (onlyNodes.length > 1) {
        const paths = onlyNodes.map((onlyNode) => {

            if (onlyNode.test) {
                return `Test: ${onlyNode.path}`;
            }
            return `Experiment: ${onlyNode.path}`;
        });
        // TODO: Report error somehow?
        return callback(new Error('Multiple tests are marked as "only":\n\t' + paths.join('\n\t')));
    }

I like the idea, but would the output show the skipped tests in the console reporter?

Only if you use -v, otherwise you just see (n skipped) (see for example this test).

@geek

This comment has been minimized.

Copy link
Member

geek commented Feb 24, 2016

@rluba one option is to show the error in a similar way to https://github.com/hapijs/lab/blob/master/lib/cli.js#L116-L118

I would like to keep it as is in regards to -i using skipped tests. I tend to use lab -v -i 44-50 or similar, and don't want to see a list of the skipped tests in that case.

@rluba

This comment has been minimized.

Copy link
Contributor Author

rluba commented Feb 24, 2016

@geek Thanks, I’ll try that. It will produce stdout output instead of stderr, but better than nothing.

@rluba

This comment has been minimized.

Copy link
Contributor Author

rluba commented Feb 24, 2016

@geek But the process.exit means that I can never cover that line according to lab: It’s covered in the "cli" tests but those don’t count against coverage because they run in a subprocess. Any suggestions?

Edit: I’ve moved the process.exit out of the big conditional to allow me to cover the latter and mark the former with a coverage exception comment.

@rluba

This comment has been minimized.

Copy link
Contributor Author

rluba commented Feb 24, 2016

So, I think I’ve changed everything you’ve mentioned. Let me know if there’s something else.

=> threw on case-sensitive file systems
@geek geek added this to the 10.0.0 milestone Feb 24, 2016
geek added a commit that referenced this pull request Feb 24, 2016
Global "only" (#524)
@geek geek merged commit 55e59e5 into hapijs:master Feb 24, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
cli.once('close', (code, signal) => {

if (signal) {
callback(new Error('Unexpected signal: ' + signal));

This comment has been minimized.

Copy link
@Marsup

Marsup Feb 24, 2016

Member

This is 8 spaces instead of 4, ESLint is missing something, unless I am...

This comment has been minimized.

Copy link
@geek

geek Feb 24, 2016

Member

Its worse than you thought: tabs!

This comment has been minimized.

Copy link
@geek

geek Feb 24, 2016

Member

looks like the new eslint ignore pattern needs to change :/ I got it

rluba added a commit to rluba/lab that referenced this pull request Feb 25, 2016
I forgot to update the readme as part of hapijs#527.

I’ve also added a section to document that one can use `.only(…)` and `.skip(…)` instead of the `options` flags.
@rluba rluba mentioned this pull request Mar 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.