feat(mocha-runner): improve Mocha support #1237
Conversation
ghost
added
the
|
bharaninb
force-pushed
the
feat/mocha
branch
from
2c9bf1d
to
a9fa291
Compare
bharaninb
changed the title
| @@ -17,6 +17,7 @@ export default class MochaTestFramework implements TestFramework { | |||
| public filter(testSelections: TestSelection[]) { | |||
| const selectedTestNames = testSelections.map(selection => selection.name); | |||
| return `var Mocha = window.Mocha || require('mocha'); | |||
| var describe = Mocha.describe; | |||
There was a problem hiding this comment.
[x] Make sure const { describe } = require('mocha'); works as well.
Based on my understanding, sometimes describe is not on the global scope.
Correct me if I am wrong.
|
|
||
| public load(config: StrykerOptions): MochaRunnerOptions { | ||
| const mochaOptions = Object.assign({}, config[mochaOptionsKey]) as MochaRunnerOptions; | ||
| let optsFileName = path.resolve(this.DEFAULT_MOCHA_OPTS); | ||
|
|
||
| if (mochaOptions && mochaOptions.opts) { |
There was a problem hiding this comment.
I notice that we always check if we have mochaOptions, but there should always be an object, right? We define it on line 13.
There was a problem hiding this comment.
Right, mochaOptions always be an object. I failed to check existing conditions, will update it.
| } | ||
|
|
||
| if (mochaOptions && mochaOptions.opts) { | ||
| throw new Error(`Could not load opts from "${optsFileName}". Please make sure opts file exists.`); |
There was a problem hiding this comment.
This looks really strange to me. Because we return on line 23, we throw an error here.
If we get an error while reading or parsing the Opts file, we will not log this error because we have no try-catch.
Could you change this to:
if (fs.existsSync(optsFileName)) {
...
} else if (mochaOptions.opts) {
this.log.error(`Could not load opts from "${optsFileName}". Please make sure opts file exists.`);
}
(The stryker-karma-runner handles an error like this by logging it instead of throwing an error)
There was a problem hiding this comment.
Based on the previous codebase, if the specified file doesn't exist, it throws an error while reading the options file. I try to achieve the same here.
Will change above statements
| @@ -116,7 +116,11 @@ export default class MochaTestRunner implements TestRunner { | |||
|
|
|||
| private additionalRequires() { | |||
| if (this.mochaRunnerOptions.require) { | |||
| this.mochaRunnerOptions.require.forEach(LibWrapper.require); | |||
| const stuffToRequire = this.mochaRunnerOptions.require | |||
There was a problem hiding this comment.
Could you rename this to modulesToRequire?
| @@ -116,7 +116,11 @@ export default class MochaTestRunner implements TestRunner { | |||
|
|
|||
| private additionalRequires() { | |||
| if (this.mochaRunnerOptions.require) { | |||
| this.mochaRunnerOptions.require.forEach(LibWrapper.require); | |||
| const stuffToRequire = this.mochaRunnerOptions.require | |||
| .map(theModule => { | |||
There was a problem hiding this comment.
Could you rename this to module?
|
Looks great! Thanks again! |
closes #1046