-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
feat: randomization support [WIP REFERENCE - DO NOT MERGE] #2301
Changes from all commits
2fe3fe4
ff75d4b
c568bef
62c6b31
a6f6e6e
5ed7851
e01cf0b
3ff40dd
75b84c5
2a4cba2
847b697
60fec82
75fe69f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,7 @@ var escapeRe = require('escape-string-regexp'); | |
var path = require('path'); | ||
var reporters = require('./reporters'); | ||
var utils = require('./utils'); | ||
var Random = require('random-js'); | ||
|
||
/** | ||
* Expose `Mocha`. | ||
|
@@ -68,6 +69,7 @@ function image(name) { | |
* - `ignoreLeaks` ignore global leaks | ||
* - `fullTrace` display the full stack-trace on failing | ||
* - `grep` string or regexp to filter tests with | ||
* - `random` for randomization of tests w/ optional seed | ||
* | ||
* @param {Object} options | ||
* @api public | ||
|
@@ -99,6 +101,9 @@ function Mocha(options) { | |
if (options.slow) { | ||
this.slow(options.slow); | ||
} | ||
if (options.random) { | ||
this.randomize(options.random); | ||
} | ||
} | ||
|
||
/** | ||
|
@@ -301,6 +306,53 @@ Mocha.prototype.ignoreLeaks = function(ignore) { | |
return this; | ||
}; | ||
|
||
/** | ||
* Enable or disable randomization of test execution order within suites. | ||
* | ||
* @param {(boolean|number|string)} seed Random seed. `seed` must be | ||
* a 32-bit unsigned integer, or a string which can convert to one. | ||
* If `false`, randomization will be globally disabled (if it was enabled | ||
* previously). | ||
* @return {Mocha} | ||
* @api public | ||
*/ | ||
Mocha.prototype.randomize = function randomize(seed) { | ||
if (!arguments.length) { | ||
throw new Error('Generate a random seed using --generate-seed on the ' | ||
+ 'command line, or Mocha.seed() in the browser.'); | ||
} | ||
if (seed !== false) { | ||
var validSeed = utils.toSafeUnsignedInteger(seed); | ||
if (isNaN(validSeed)) { | ||
throw new Error('Invalid random seed. Expected unsigned 32-bit ' | ||
+ 'integer or value which can convert.'); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a particular use case for supporting numbers-in-strings directly in the randomization function? I would think it would be on the caller to do the conversion. Having to specify number conversion behavior in the randomization function itself complicates the API, and for little benefit given that it could be handled separately. And if it were handled separately, it would be easier to update in isolation if need be, or to test, etc. The one place I can think of it being needed in Mocha is the CLI. Ideally, I'd say the string to seed conversion should be in its own utility function so that in the event it is needed in more than one place it could be reused, so that it can be tested separately from testing the stuff that uses it or the randomization it feeds into, and so we can consider exposing it to users/wrappers of Mocha if they want to get the same conversion method that Mocha uses for the seed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right; string to integer conversion is necessary for the CLI only. At some point, we'll have to do it. I can move the implementation elsewhere. |
||
|
||
var engine = Random.engines.mt19937().seed(validSeed); | ||
this.options.randomConfig = { | ||
shuffleTests: function shuffleTests(tests) { | ||
Random.shuffle(engine, tests); | ||
return tests; | ||
}, | ||
seed: validSeed, | ||
hex: '0x' + Number(validSeed).toString(16).toUpperCase() // for display | ||
}; | ||
} else { | ||
delete this.options.randomConfig; | ||
} | ||
return this; | ||
}; | ||
|
||
/** | ||
* Generate a random seed. | ||
* @returns {string} Hexadecimal string | ||
*/ | ||
Mocha.seed = function seed() { | ||
var engine = Random.engines.mt19937() | ||
.autoSeed(); | ||
return utils.toHex(Random.integer(0, utils.MAX_SAFE_INTEGER)(engine)); | ||
}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It'd be nice to have a comment explaining why we use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That comment would be something like "go read the docs for this other library". 😜 Basically, you can't get a value out of the engine unless it's seeded. So if we want a seed, then we should randomly generate one. And if we're randomly generating a seed, why not use the random number generator..? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @boneskull, I don't think you understood @ScottFreeCode's question. The question was if Mocha.seed = function seed() {
return Random.engines.mt19937().autoSeed();
}; The answer seems to be no, since There's also a minor inaccuracy in the JSDoc ( Altogether the code should IMO look like this: /**
* Generate a random seed.
* @returns {number} Unsigned 53-bit integer
*/
Mocha.seed = function seed() {
/*
* Create an auto-seeded random number generator.
* We can't use `Random.generateEntropyArray` (used by `autoSeed`)
* for this function since our seed is an integer.
*/
var generator = Random.engines.mt19937().autoSeed();
/*
* Return a random integer that can be used as a seed, using the
* random number generator we just created.
*/
return Random.integer(0, MAX_SAFE_INTEGER)(generator);
}; |
||
|
||
/** | ||
* Enable global leak checking. | ||
* | ||
|
@@ -506,6 +558,10 @@ Mocha.prototype.run = function(fn) { | |
if (options.useColors !== undefined) { | ||
exports.reporters.Base.useColors = options.useColors; | ||
} | ||
if (options.randomConfig) { | ||
runner.randomConfig = options.randomConfig; | ||
} | ||
|
||
exports.reporters.Base.inlineDiffs = options.useInlineDiffs; | ||
|
||
function done(failures) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this number coming from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather get the number from the JS API or, if not available, an npm module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From https://www.npmjs.com/package/random-js#distributions
Number.MAX_SAFE_INTEGER
is the same value.EDIT:
Number.MAX_SAFE_INTEGER
is only available in ES2015 and later.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, sorry.
0x1fffffffffffff
is the value ofNumber.MAX_SAFE_INTEGER
ifNumber.MAX_SAFE_INTEGER
is implemented. But we're not guaranteed that, of course, and I didn't want to pull in some polyfill. I'll add a comment.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dasilvacontin Would prefer not to
npm install --save max-safe-integer
(don't know if that exists; probably does) if I can help it.