Add randomization support #1512

TimothyGu · 2015-01-23T05:58:36Z

Big Fat Warning

This is a WIP. I just wanted to throw the idea out there and see if there's any feedback before I start writing tests and docs.

How to use it?

The array shuffling is done with the knuth-shuffle-seeded, my fork of knuth-shuffle that supports seeding.

mocha --randomization tests
mocha -a tests
mocha -a tests:mySeed123ee456eeed

{
  randomizeMode: 'test'
, randomizeSeed: 'mySeed'
}

If your test needs to be tested in a particular order, add noRandom:

describe.noRandom('my test', function () {
  [...]
})

noRandom plays nicely with other modifiers (although I admit the implementation is a little ugly):

describe.noRandom.only()
describe.only.noRandom()
describe.noRandom.skip()
describe.skip.noRandom()
xdescribe.noRandom()

Current state

Currently it only supports test-level randomization (mode === 'tests'), which means that only the order of tests within a suite is randomized.

TODO

tests
docs
suite-level randomization (might be added after this PR)

Fixes #902.

TimothyGu · 2015-01-23T06:17:50Z

I'll take a look at Node.js 0.8 later (which is causing the Travis error).

Fixes mochajs#902.

travisjeffery · 2015-01-29T23:56:45Z

man those added apis are nastyyyy :S

travisjeffery · 2015-01-29T23:57:12Z

i'm not really keen on this being in mocha

TimothyGu · 2015-01-30T00:00:21Z

@travisjeffery you mean those only.noRandom and noRandom.only? Yes I would agree so. If you have any other way of doing it I would be happy to reimplement it.

dasilvacontin · 2015-01-30T00:00:29Z

It would be desirable to discuss/design the API before hand in the issue. :/

TimothyGu · 2015-01-30T00:01:33Z

@dasilvacontin well I did comment on the issue but nobody responded: #902 (comment)

dasilvacontin · 2015-01-30T00:03:54Z

@TimothyGu, sorry, I'm quite busy until mid-Feb, and since I tend to not open slack anymore I miss a lot of convs/threads.

TimothyGu · 2015-01-30T00:07:41Z

@dasilvacontin It's all good, just wanted to point out my comment

travisjeffery · 2015-01-30T00:12:01Z

it's not the api, i just don't think it's needed in mocha

TimothyGu · 2015-01-30T00:16:37Z

@travisjeffery You mean the feature? Well #902 is tagged as "PR PLEASE"

dasilvacontin · 2015-01-30T00:16:46Z

@travisjeffery pretty much everyone in the issue agreed it would be helpful, even @boneskull. At the very least we could leave it in a plugin.

boneskull · 2015-01-30T00:18:04Z

@travisjeffery It's probably not something I'd use, but many people want this.

travisjeffery · 2015-01-30T00:28:51Z

all kinds of people want kinds of stuff, not really a great argument for anything imo. it would be sweet if we could add enable this feature as an external plugin however

TimothyGu · 2015-01-30T03:07:26Z

@travisjeffery

Well again I just implemented it because it looks doable, fun, requested, and tagged as "PR PLEASE." If you disagree with doing it in Mocha, I would seriously think that you guys should clean up the issues tagged as PR PLEASE first, AND possibly close #902.

Personally, I do not care about this feature. It is likely that I will not even ever use this feature. I just did it for the good of the community, and as it looks like that the feature is requested by multiple people and looks useful.

I am not inclined to do further work on it if it is not appreciated, nor am I motivated enough to turn it into a plugin (even less so when there is literally zero documentation on doing so anywhere I could find).

TimothyGu · 2015-01-30T03:14:26Z

Plus, if you think randomization is not helpful in the main Mocha distribution, why are useless things (let's be practical) like this or that even included in the standard Mocha distribution?

TimothyGu · 2015-01-30T03:17:26Z

I said:

even less so when there is literally zero documentation on doing so anywhere I could find

Ah now I understand why, because the plugin framework doesn't even exist.

boneskull · 2015-01-30T03:54:07Z

why are useless things (let's be practical) like this or that even included in the standard Mocha distribution?

No time to pull them out. Extracting the reporters (especially these reporters) from the core is on the TODO list.

dasilvacontin · 2015-01-30T03:55:48Z

There was even an initiative to make framework-independent reporters, but guess how that is going. :)

boneskull · 2015-01-30T03:56:06Z

it would be sweet if we could add enable this feature as an external plugin however

As @TimothyGu said, no such thing exists. So in the meantime, I'm happy to add one of the most requested features, which is why this was tagged PR PLEASE.

boneskull · 2015-01-30T04:01:56Z

all kinds of people want kinds of stuff, not really a great argument for anything imo.

If one random dude wants one random thing, then probably not. This is not one of those, however.

If I had more time to dedicate to this, I'd try to give the most requested features attention, and add them, if I agree they belong somewhere in this framework. But I don't have the time, and we don't have the luxury of saying x feature should be in core, and y should be a plugin, because we have no plugin API.

So we have @TimothyGu who takes the time to do it. I haven't even had a chance to look at the PR, but if it looks good, I'd like to merge it.

boneskull · 2015-01-30T04:13:55Z

@travisjeffery

man those added apis are nastyyyy :S

This is not necessary.

dasilvacontin · 2015-01-30T04:28:38Z

Same thoughts here, @boneskull. I tend not to express myself much, maybe I should do it more often.

dasilvacontin · 2015-01-30T04:31:25Z

We definitely appreciate the work you've put into the PR, @TimothyGu, in case it is not clear.

Once it looks good, we will merge it into core. And once we have a stable Plugin API, we will debate whether to export it into a plugin.

TimothyGu · 2015-01-30T04:56:28Z

@boneskull and @dasilvacontin, I appreciate your constructive comments.

TimothyGu · 2015-02-14T18:46:07Z

@dasilvacontin @boneskull ping? I will rebase the PR if you guys can take a look at it first.

dasilvacontin · 2015-02-14T19:04:42Z

lib/suite.js

+  */
+
+Suite.prototype.enableRandomize = function(enabled){
+  if (arguments.length === 0) return this._enableRandomize;


It feels to me that executing this.enableRandomize() in a suite should enable randomize, but it doesn't, right?

Nvm, without arguments it's used as a getter... I think it would be bad to see .enableRandomize() around being used as a getter – not logical at all.

dasilvacontin · 2015-02-14T19:09:04Z

The PR is lacking tests asserting that enableRandomize and noRandom functions are actually doing their job setting booleans.

It would also be nice to have a test suite that we run with a fixed seed, and check if the order of test execution is correct. And another one with a non-defined seed that just checks the tests are not run in the order they are described (it can still fail, but the probability is very low).

boneskull · 2015-02-14T19:09:46Z

@TimothyGu Sorry busy as hell atm. Should have some time tomorrow

dasilvacontin · 2015-02-14T19:12:00Z

Also, are suites inheriting ._enableRandomize? This should be asserted in the tests as well.

TimothyGu · 2015-02-14T19:12:47Z

@dasilvacontin said:

The PR is lacking tests asserting that enableRandomize and noRandom functions are actually doing their job setting booleans.

I know, hence the TODOs in the first comment of the thread.

dasilvacontin · 2015-02-14T19:16:10Z

@TimothyGu Oh sorry, I didn't re-read that part and just went on giving overall review of the PR.

boneskull · 2015-02-16T09:18:56Z

lib/runner.js

+
+Runner.prototype.randomize = function(mode, seed){
+  debug('randomize mode: "' + mode + '"; seed: ' + seed);
+  if (mode !== 'tests') return this;


Is this temporary?

Yes, as for now only tests can be randomized.

dasilvacontin · 2015-04-02T18:55:17Z

Any update, @TimothyGu?

TimothyGu · 2015-04-30T17:05:44Z

@dasilvacontin I've been fairly busy with other stuff recently, but I'll take a look at the suggestions soon (I hope).

dasilvacontin · 2015-04-30T17:17:44Z

No worries. :)

Le 30 avr. 2015 à 19:05, Timothy Gu notifications@github.com a écrit :

@dasilvacontin I've been fairly busy with other stuff recently, but I'll take a look at the suggestions soon (I hope).

—
Reply to this email directly or view it on GitHub.

boneskull · 2015-06-03T18:25:27Z

@TimothyGu not saying anyone will do this, but I'd like to get some traction on it; do you mind if me or someone else grabs your branch and makes changes?

TimothyGu · 2015-06-03T21:18:15Z

@boneskull not at all, as long as maybe I'm credited for an initial implementation.

jbnicolai · 2015-06-05T11:04:56Z

@boneskull 👍 if you'd like to pick this up. Great feature to have :)

boneskull · 2016-06-09T03:41:01Z

I'm going to see what I can do with this.

boneskull · 2016-06-09T04:10:58Z

@TimothyGu I will need to close this PR and create a new one, but your name will be on at least one commit here.

TimothyGu · 2016-06-09T04:35:47Z

Alright, thanks.

TimothyGu force-pushed the rand branch from de52929 to 3f745e2 Compare January 23, 2015 06:13

TimothyGu force-pushed the rand branch 2 times, most recently from affba74 to 22e2c13 Compare January 23, 2015 06:25

Add randomization support

de522d7

Fixes mochajs#902.

TimothyGu force-pushed the rand branch from 22e2c13 to de522d7 Compare January 23, 2015 06:50

dasilvacontin reviewed Feb 14, 2015
View reviewed changes

boneskull reviewed Feb 16, 2015
View reviewed changes

boneskull added the pr-needs-work label Feb 16, 2015

jbnicolai force-pushed the master branch 3 times, most recently from 2f458ab to 2952eca Compare July 5, 2015 10:25

boneskull self-assigned this Jun 9, 2016

boneskull closed this Jun 9, 2016

boneskull removed the pr-needs-work label Dec 12, 2017

boneskull removed their assignment Dec 12, 2017

Add randomization support #1512

Add randomization support #1512

Conversation

TimothyGu commented Jan 23, 2015

Big Fat Warning

How to use it?

Current state

TODO

TimothyGu commented Jan 23, 2015

travisjeffery commented Jan 29, 2015

travisjeffery commented Jan 29, 2015

TimothyGu commented Jan 30, 2015

dasilvacontin commented Jan 30, 2015

TimothyGu commented Jan 30, 2015

dasilvacontin commented Jan 30, 2015

TimothyGu commented Jan 30, 2015

travisjeffery commented Jan 30, 2015

TimothyGu commented Jan 30, 2015

dasilvacontin commented Jan 30, 2015

boneskull commented Jan 30, 2015

travisjeffery commented Jan 30, 2015

TimothyGu commented Jan 30, 2015

TimothyGu commented Jan 30, 2015

TimothyGu commented Jan 30, 2015

boneskull commented Jan 30, 2015

dasilvacontin commented Jan 30, 2015

boneskull commented Jan 30, 2015

boneskull commented Jan 30, 2015

boneskull commented Jan 30, 2015

dasilvacontin commented Jan 30, 2015

dasilvacontin commented Jan 30, 2015

TimothyGu commented Jan 30, 2015

TimothyGu commented Feb 14, 2015

dasilvacontin Feb 14, 2015

Choose a reason for hiding this comment

dasilvacontin Jun 9, 2016

Choose a reason for hiding this comment

dasilvacontin commented Feb 14, 2015

boneskull commented Feb 14, 2015

dasilvacontin commented Feb 14, 2015

TimothyGu commented Feb 14, 2015

dasilvacontin commented Feb 14, 2015

boneskull Feb 16, 2015

Choose a reason for hiding this comment

TimothyGu Feb 16, 2015

Choose a reason for hiding this comment

dasilvacontin commented Apr 2, 2015

TimothyGu commented Apr 30, 2015

dasilvacontin commented Apr 30, 2015

boneskull commented Jun 3, 2015

TimothyGu commented Jun 3, 2015

jbnicolai commented Jun 5, 2015

boneskull commented Jun 9, 2016

boneskull commented Jun 9, 2016

TimothyGu commented Jun 9, 2016