mochajs / mocha

☕️ simple, flexible, fun javascript test framework for node.js & the browser
https://mochajs.org
MIT License
22.63k stars 3.02k forks source link

🚀 Feature: --order option for random test order #902

Open sarenji opened 11 years ago

sarenji commented 11 years ago

An --order option would allow people to uncover order dependencies. The three options would be --order random, --order random:seed, and --order default. Each randomized suite outputs the seed it used.

RSpec implements this, but their default order is random. Mocha doesn't have to do that. Some details on their --order parameter are here: http://blog.davidchelimsky.net/2012/01/04/rspec-28-is-released/

What do you think?

tj commented 11 years ago

meh, pretty easy to avoid cross-test dependencies without help of tooling

yanovich commented 10 years ago

While it is easy to avoid cross-test dependencies without help of tooling, it is also easy to add such a dependency in a test suite without noticing. A bug in the test suite usually translates to a bug in a tested system. These bugs are hard to trace, as the code is supposedly test-covered.

Testing for absence of cross-test dependencies is impossible without tooling.

@visionmedia, please reconsider.

trshafer commented 10 years ago

+1 @yanovich. I would use a random order option which outputs a seed number. This would be very useful in a CI environment.

@visionmedia, mongoose models provide an easy example of cross-test dependencies. mongoose.model 'User', UserSchema adds a model onto the array of mongoose.models. So it's possible to create a file which relies on the user model being loaded in mongoose.models. Take Comment.find().populate('_user').exec(cb) as an example. If the user test runs before the comment test, this will execute fine, because presumably require('./models/user') (or something), has loaded the User model into mongoose.models. But if the comment test executes before the user test you'll get this error Schema hasn't been registered for model "User". This could happen in production when the comment api runs before the user api and the comment file didn't know it had a cross file dependency.

It is possible to still have the production problem with the test working if the test file has require('./models/user') (or whatever) and that loads the user into mongoose.models. However having a random order would be one more useful tool to discover potential problems like this.

I hope articulated that well. Looking forward to hearing your thoughts.

tj commented 10 years ago

sorry, I think it's major overkill, mocha is bloated enough as-is. If there was a lot more interest then perhaps it would be worth the maintenance burden.

trshafer commented 10 years ago

Thanks for thinking about it.

timruffles commented 10 years ago

Like most things in code, it's easy for people who know to avoid doing this intentionally. It's harder to avoid doing it unintentionally. And if you don't know it's even a problem (i.e mixed experience teams) it's downright likely to happen :)

Seems like quite a few ppl are interested in it (and lots think it's one of the best features of minitest). If it'd get merged I'm happy to implement.

MaerF0x0 commented 10 years ago

+1 interested.

Rush commented 10 years ago

Would be nice to have! I found my tests to fails by renaming file names, ugh.

crismali commented 10 years ago

+1 this is important

OliverJAsh commented 10 years ago

:+1:

shlima commented 10 years ago

:+1:

syrnick commented 10 years ago

+1 This is a pretty big deficiency.

rspec semantics are pretty solid: you can pass an order seed, or it can pick it at random. If it picks the seed at random, it prints it out, so it's easy to reproduce.

syrnick commented 10 years ago

It's often not that easy to avoid cross-test dependencies. Sometimes due to unforeseen global interactions, sometimes out of convenience. I suspect more than 50% of projects that use mocha would see test failures if the order was randomized. Here's a couple examples that appear to rely on the order of the test execution:

https://github.com/visionmedia/mocha/blob/master/test/hook.async.js#L95 https://github.com/visionmedia/superagent/blob/master/test/node/not-modified.js#L31

These two are listed as exemplar test suites on http://visionmedia.github.io/mocha/ and I didn't spend much time looking for issues.

boneskull commented 10 years ago

I'll reopen this. I think it'd be helpful. While there are ways to determine cross-test deps w/o tooling, if we can automate that, it'd save people time.

After toying with this a bit, it appears non-trivial due to the hierarchical nature of Suites. Tests are run by recursing into Suites. To run Tests randomly, we'd have to enumerate them, randomize them, then work backwards.

This would cause before() and after() Hooks to be somewhat meaningless as they would get executed n times per n tests in a Suite (or rather, in the worst case, but only if we're careful), as we continually change contexts. Sounds like it'll incur a performance penalty.

Using random seeds and reporting auto-generated seeds seems trivial, however, reporters may need to know about this information, so that requires implementation(s) in the reporters.

Of course, I'm assuming what I've described here is what's being asked for. A feature like this needs a specification.

Other options include "randomize Suites" or "randomize tests within Suites" or some combination of the two. Practically speaking, this means that once you're in a describe() block A, you cannot execute tests in any parent or sibling describe() block B until all of the tests in A have been run (which looks to be a much more straightforward implementation, and won't cause hinkiness with before()/after()).

syrnick commented 10 years ago

What I am (and I think others are) asking for is the simplest of the options:

I don't think there's much value in shuffling things at the intermediate levels.

Certainly a hack, but works for the lowest level https://github.com/syrnick/mocha/compare/random_order?expand=1&w=0

mocha - fail
connect - pass
superagent - fail
express - pass** 
websocket.io - pass (can't tell for sure)

\ I got 2 intermittent failures out of 100 runs of the whole test suite either way.

boneskull commented 10 years ago

OK, that's certainly eaiser to implement!

I was looking at the seedrandom lib for this; use the pass option.

Would accept PR.

syrnick commented 10 years ago

I'll likely clean up that code and adjust the test suite over the next few days. Is underscore too heavy of a dependency for this? I could likely use something light like this: http://stackoverflow.com/questions/6274339/how-can-i-shuffle-an-array-in-javascript.

jbnicolai commented 10 years ago

@boneskull I support your decision to reopen this. :+1:

Other options include "randomize Suites" or "randomize tests within Suites" or some combination of the two.

Seems more than good enough for me. No need to recurse all the way down, enumerate and shuffle.

timruffles commented 10 years ago

Great to hear this is going in.

I wonder if rspec has handled the recursive shuffle? Might be worth looking at their code?

On Tuesday, 26 August 2014, Joshua Appelman notifications@github.com wrote:

@boneskull https://github.com/boneskull I support your decision to reopen this. [image: :+1:]

Other options include "randomize Suites" or "randomize tests within Suites" or some combination of the two.

Seems more than good enough for me. No need to recurse all the way down, enumerate and shuffle.

— Reply to this email directly or view it on GitHub https://github.com/visionmedia/mocha/issues/902#issuecomment-53482124.

boneskull commented 10 years ago

@syrnick I would not want to accept a PR with such a large dependency, and instead use seedrandom. Without it, I'm not sure how you're going to support seeding. seedrandom allows you to specify a seed or not, and if you don't, it will return a seed to you. Then we could display it to the user and allow them to specify it, a la RSpec.

boneskull commented 10 years ago

@syrnick Mind you that if you do generate seeds, they may not be "displayable" without passing them off to reporters. I'm not very familiar with the reporting architecture, so I couldn't tell you for sure, or what to do...

saks commented 10 years ago

+1

schoblaska commented 10 years ago

I haven't looked at the implementation, but +1 to randomly-ordered test execution by default being super important.

boneskull commented 10 years ago

@syrnick Please let me know if you intend to do this, thanks.

syrnick commented 10 years ago

I'm happy to do that, but I don't have an immediate ETA.

parkr commented 10 years ago

:+1:, you guys still need help with a PR?

dasilvacontin commented 10 years ago

Indeed, no one seems to have started working on this.

TimothyGu commented 9 years ago

First, it looks like a Fisher-Yates shuffle would do the job here.

Second, I'd rather have --order random, --order random-suites, and --order default as the three arguments, with an optional :<seed>.

demisx commented 9 years ago

+1. Just found a bug that would've shown up a long time ago if tests were randomized. Similar to how RSpec supports it.

neontapir commented 9 years ago

Here's some code that illustrates the usefulness of random test ordering. While there are simpler examples, this is one I just encountered during a TDD demo. If you reverse the order of the tests, the first test always fails.

game.js:

var express = require('express');
app = exports.app = express();

var sum = 0;

app.post('/bowl/:pins', function(req,res) {
    var score = parseInt(req.params.pins);
    console.log('Bowled ' + score);
    sum += parseInt(req.params.pins);
});

app.get('/score', function(req,res) {
    console.log('Sum: ' + sum);
    res.send(sum + '');
});

app.listen(process.env.PORT || 3000);

test\gameTest.js:

var request = require('supertest'),
    should = require('should'),
    game = require('../game.js').app;

describe('a game of bowling', function() {
    describe('a gutter game', function() {
        it('should score 0', function(done){
            request(game).get('/score').expect(200, '0', done);
        });
    });

    describe('a single pin game', function() {
        it('should score 20', function(done){
            for(var i = 0; i < 20; i++) {
                request(game).post('/bowl/1').expect(200, done);
            }
            request(game).get('/score').expect(200, '20', done); 
        });
    });
});
elicwhite commented 9 years ago

I would love to have this.

tomconroy commented 9 years ago

:+1:

fishermand46 commented 9 years ago

Once you get a few globals involved (this is Javascript, remember), start stubbing out server calls, and inserting/removing things from the DOM in your tests, it is very easy to add order-dependency. Randomizing the test order would help discover these earlier rather than later. :+1:

tsiege commented 9 years ago

:+1:

jvennix-rmn commented 9 years ago

:+1:

baio commented 9 years ago

+1

alxndr commented 9 years ago

Random order by default, with optional seed to recreate ordering, would be a great feature to have.

NicolasJacob commented 9 years ago

+1 to have it, my tests sometime fail when run in random order...

In the mean time, unix to the rescue (Unfortunately, random seed no supported):

mocha `ls -1 test/*.js | sort --random-sort `
danielabar commented 9 years ago

Was googling for what order mocha runs tests in and found this. In absence of randomization, what is the default run order? Is it always the order in which the tests appear physically in the file?

jmnsf commented 9 years ago

:+1:

travisjeffery commented 9 years ago

@danielabar yeah they'll be in-order they appear in the file.

ryoqun commented 9 years ago

@NicolasJacob well, random seed is actually possible to some extent, btw. :)

$ seq 10 | shuf --random-source=<(yes 2883)
1
7
3
4
6
2
10
5
9
8
macalinao commented 8 years ago

https://github.com/bahmutov/rocha works for this.

jgwmaxwell commented 8 years ago

@boneskull whilst this is an old issue, is the PR Please label still valid? If so I'll get something contributed in the next day or so.

danielstjules commented 8 years ago

I think in the pursuit of eventually trying to keep the mocha core minimal, the team might be hesitant in introducing many new features. The next major release of mocha has the goal of having a pluggable interface.

Might I suggest just using https://github.com/bahmutov/rocha if it works?

macalinao commented 8 years ago

Awesome sauce

mren commented 8 years ago

What do you mean by pluggable interface? Will it be possible to introduce a randomized testing order via this interface?

sulabhjain commented 8 years ago

+1 for the feature request

dasilvacontin commented 8 years ago

@sulabhjain, previous and following supporters, please use the +1 reaction instead.

boneskull commented 8 years ago

Progress in this branch.