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

Implement the only modifier. #204

Closed
wants to merge 3 commits into from
Closed

Conversation

lijunle
Copy link
Contributor

@lijunle lijunle commented Nov 12, 2015

DO NOT MERGE

Resolve #132

I hack the test count number to show the right number and passed cases number. From my understand, such information should be pass through the promise chain.

Besides, ava.serial.only has no support in this PR yet.

/cc @vdemedes @sindresorhus


Screenshot:

screenshot_2015-11-12_23-48-19

})
.then(function () {
return eachSeries(tests.after, self._runTest.bind(self));
})
.catch(noop)
.then(function () {
// HACK the test count number if only modifier is shown
stats.testCount = tests.only.length ? tests.only.length : stats.testCount;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The variable testCount is maintained by the addTest, addOnlyTest, addSerialTest function, which I think that is not suitable. A better way is to return such information (testCount, failCount) from the promise, then calculate the final stats from at the end of the chain.

However, it involves more code change if doing so. Any ideas?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's perfectly fine, I wouldn't call it a hack.

@sindresorhus
Copy link
Member

Awesome @lijunle :) Both @vdemedes and I are traveling, so we probably won't be able to review this until monday.

// @jamestalmage

@lijunle
Copy link
Contributor Author

lijunle commented Nov 12, 2015

@sindresorhus That is OK, please keep in mind that, there is a hack. I am not going to merge until resolve the hack with a good way (unless a direction).

@@ -203,16 +209,21 @@ Runner.prototype.run = function () {
}
})
.then(function () {
return self.serial(tests.serial);
return tests.only.length ? self.concurrent(tests.only) : [];
Copy link
Contributor

Choose a reason for hiding this comment

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

it can be just return self.concurrent(tests.only), since tests.only defaults to [].

@vadimdemedes
Copy link
Contributor

Everything looks good so far, I pointed out some things in the diff. One question I have in mind, should only tests be executed serially on in parallel? Btw, mocha allows only one only test, so maybe we should also do that? And it would solve the serial vs parallel question too.

@lijunle
Copy link
Contributor Author

lijunle commented Nov 17, 2015

One question I have in mind, should only tests be executed serially on in parallel?

My question is no. If the guy wants to do so, he should do ava.serial.only. However such API is not implemented in this RP.

Btw, mocha allows only one only test, so maybe we should also do that?

Mocha allow several only modifiers. For example, if there are 10 test cases, and 3 of them are marked as only, those 3 test cases will be run.

@vadimdemedes
Copy link
Contributor

Mocha allow several only modifiers. For example, if there are 10 test cases, and 3 of them are marked as only, those 3 test cases will be run.

I guess I was running some out-dated version then.

@vadimdemedes
Copy link
Contributor

@lijunle Could you please rebase your PR (to pick up latest fixes for windows tests) and address comments in the diff? Happy to merge this ASAP.

@lijunle
Copy link
Contributor Author

lijunle commented Nov 17, 2015

OK, do you want to modify the comment about the hack?

@vadimdemedes
Copy link
Contributor

Yeah, the comment is not really needed and this - https://github.com/sindresorhus/ava/pull/204/files#r45035490

- Hack the test count number if only modifier is shown.
@lijunle
Copy link
Contributor Author

lijunle commented Nov 17, 2015

Rebased, let us wait for the CI. 😸


However, although I removed the comment, I still consider that is a hack.

The stats should be collected from _runTestWithHooks function and pass back to the promise chain in the Runner.prototype.run function. At the end of the promise chain, calculate the final stats from the input.

Code like this:

Runner.prototype.run = function () {
    var self = this;
    return eachSeries(tests.before, this._runTest.bind(this)) // resolve with { pass: 1, failed: 2, skipped: 3 }
        .then(function (stats) {
            return Object.assign(stats, self.serial(tests.serial)); // i.e. { pass: 2, failed: 3, skipped: 4 }
        })
        .then(function (stats) {
            return Object.assign(stats, self.concurrent(tests.concurrent)); // object with { pass, failed, skipped }
        })
        .then(function (stats) {
            return Object.assign(stats, eachSeries(tests.after, self._runTest.bind(self))); // object with { pass, failed, skipped }
        })
        .then(function (stats) {
            self.stats.passCount = stats.pass; // calculate from the input of promise chain.
            return stats; // external check could depends on return value of run function, more stable. 
        });
};

@vadimdemedes
Copy link
Contributor

The refactoring and cleanup work is ahead, so don't worry about this 1 line ;)

@lijunle
Copy link
Contributor Author

lijunle commented Nov 17, 2015

I understand. It is up to you to design the pattern, that is just my suggestion.

@jamestalmage
Copy link
Contributor

@lijunle

I was actually taking a stab at this earlier when I got pulled away to some other stuff.
Since you are pretty far down the road, I am holding off.

I just squashed my work and pushed it up: jamestalmage@4568347

Use it if it helps.

My other thoughts.

  1. I would really like to avoid the Mocha behavior of only allowing a single test in exclusive mode. I want to be able to put only on however many tests I chose and have them be the ones that run. This is especially important since we don't have grouping yet.
  2. I like your idea of test.serial.only, etc.
  3. I think creating stats at the beginning is making all this hard. Just put flags in each individual test and use this.tests.concurrent.filter to create the stats and get at the tests you want (see the diff I linked).

Good work! And thanks for the help.

@vadimdemedes
Copy link
Contributor

@lijunle Thank you!

@sindresorhus
Copy link
Member

@jamestalmage Any improvements welcome in a follow-up pull request ;)

@vadimdemedes vadimdemedes mentioned this pull request Nov 17, 2015
@jamestalmage
Copy link
Contributor

Yeah, sorry!
I've got a lot going on today and am lagging way behind the conversation.

@lijunle lijunle deleted the only-test branch November 17, 2015 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants