Refactor Reporter tests#3272
Conversation
|
|
|
great effort, thanks. I'll review this when I get a minute |
| callback(test); | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
How about throwing an exception here to prevent typo or new runStr?
| callback(); | ||
| } | ||
| }; | ||
| } else if (runStr === 'pending test' || runStr === 'pass' || runStr === 'fail' || runStr === 'end' || runStr === 'suite' || runStr === 'suite end' || runStr === 'test end') { |
There was a problem hiding this comment.
It's hard to read and makes a verbose diff when we add a new case.
What do you think about format it like:
} else if (
runStr === "pending test" ||
runStr === "pass" ||
runStr === "fail" ||
runStr === "end" ||
runStr === "suite" ||
runStr === "suite end" ||
runStr === "test end"
) {
boneskull
left a comment
There was a problem hiding this comment.
Hey, thanks! Awesome work here. I have a few suggestions. Can you take a look?
| var reporters = require('../../').reporters; | ||
| var Doc = reporters.Doc; | ||
|
|
||
| var runnerEvent = require('./helpers.js').runnerEvent; |
There was a problem hiding this comment.
I'm not sure what to call this, but runnerEvent isn't descriptive enough.
Also, I'm not sure runner needs to be created as an empty object in beforeEach...
Instead, maybe this could just be a factory function for a runner object? It would then create .on and .once as appropriate.
Then call it something like createMockRunner
| */ | ||
| function runnerEvent (runStr, ifStr1, ifStr2, ifStr3, arg1, arg2) { | ||
| var test = null; | ||
| if (runStr === 'start' || runStr === 'pending' || runStr === 'end') { |
There was a problem hiding this comment.
don't be afraid to use a switch in here, or an object lookup.
|
|
||
| function createElements (argObj) { | ||
| var res = []; | ||
| for (var i = argObj.from; i <= argObj.to; i += 1) { |
| stdout.push(string); | ||
| }; | ||
|
|
||
| expectedTitle = 'some title'; |
There was a problem hiding this comment.
It doesn't make a lot of sense to declare all this above then define it here, does it? Can we not just define it all above?
| callback(test); | ||
| } | ||
| }; | ||
| // var test = { |
|
Ok, hope that is an improvement, did just what you said, well I believe I did |
|
@jmuzsik LGTM! Thanks a ton. |
* Closes mochajs#3260 * refactor-better test harness for reporters tests' * refactor-better test harness for reporters tests' * refactor - some additional work * refactor - helper func * refactor - helper func * refactor - helper func * fix - improvements to helper functions
* Closes mochajs#3260 * refactor-better test harness for reporters tests' * refactor-better test harness for reporters tests' * refactor - some additional work * refactor - helper func * refactor - helper func * refactor - helper func * fix - improvements to helper functions
Description of the Change
In reference to issue: #3260
I did my best at refactoring the code. Though the part of the test files that was quite obviously capable to become more modular was the runner events. I could make all of those calls from one function, ie:
Otherwise I tried to make variables that I noticed were repeatedly called throughout singular files to be called only one time rather then the continual
var something = 'something'over and over.Alternate Designs
I believe that it can still be worked on. It is not perfect. The
runnerEventfunction could be more beautiful/intuitive. Some tests can make more sense. BaseReporter tests run but it is not obvious, I have no idea how i'd implement that.Why should this be in core?
Quite a bit less lines of code for the same functionality.
Benefits
Less intensive use of memory and variable calls, etc. More modular code.
Possible Drawbacks
I had to alter quite a bit of code that a lot of people put lots of time into. There are no errors atm, and I expect there not to be.
Applicable issues
Not that I know of
Open to doing more if there is a recommendation.