chore(launcher): reorganize launcher #1485
Conversation
|
hey @juliemr can you review when you get a chance? It's pretty experimental so feel free to say the current way of doing launcher/runner is good and leave it alone, or just grab some pieces that you like from this huge PR. |
hankduan
force-pushed
the
refactorLauncher
branch
from
14e9e1d to
2bdf968
Compare
| * start a new process that calls on '/runFromLauncher.js'. | ||
| * | ||
| * @constructor | ||
| * @param {object} task Task to run. |
There was a problem hiding this comment.
Add param annotations for the other parameters.
There was a problem hiding this comment.
Oh, I didn't change any comments yet. I'll do that as a next iteration, but I just want to see if you like the general feel of the refactor or you think it's excessive.
There was a problem hiding this comment.
Gotcha - will keep my feedback on that level then.
|
I think these are all good changes - let's update the comments and docs. |
| @@ -50,6 +50,7 @@ var optimist = require('optimist'). | |||
| describe('stackTrace', 'Print stack trace on error'). | |||
| describe('params', 'Param object to be passed to the tests'). | |||
| describe('framework', 'Test framework to use: jasmine, cucumber or mocha'). | |||
| describe('testResultOutput', 'Path to save JSON test result'). | |||
There was a problem hiding this comment.
We'll need to add to docs/referenceConf - also, I'm not sure about this name. What about resultJsonOutputFile?
|
Alright, in that case, I'll finalize the changes tomorrow with: Thanks for the review! |
|
Fixed comments, docs, etc. |
|
LGTM |
|
@juliemr I just got cucumber to work too. Can you review that file too before I submit? |
| // Add a listener into cucumber so that protractor can find out which | ||
| // steps passed/failed | ||
| var addResultListener = function(formatter) { | ||
| var handleStepResultEvent = formatter.handleStepResultEvent; |
There was a problem hiding this comment.
for clarify, can you rename to 'originalHandleStepResultEvent'?
|
Couple style nits, then LGTM |
hankduan
force-pushed
the
refactorLauncher
branch
from
3519046 to
922de7a
Compare
|
merged in 55a91ea |
---experimental---
(motivated by #1449)
After months of commits/changes to runner and launcher code, I think it needs a little clean up. This change has no feature changes or significant output changes.
motivation:
BEFORE: running single test vs forked (sharded/multibrowser) use completely different code paths in terms of error handling, what runner returns, adding listeners, etc, leading to issues like fix(launcher): exit code is always 100 for sharded and 1 for nonsharded ... #1448. Furthermore, at the current state, it means that whenever we need to process the output (i.e. store test output into json file), we need to write different logic to deal with the 2 paths.
NOW: launcher always calls runnerfork with a parameter to fork or not.
runnerfork.run()always returns a promise with results of the run, and reject on errorsBEFORE: Some files contains multiple not-very-related classes that are growing pretty big (i.e. launcher contains reporter, launcher, runnerfork). Moreover, these classes are mutating shared variables and these classes would undoubtedly become more intertwined in the future, leading to future problems
NOW: broke launcher into launcher, logger, runnerfork
BEFORE: It is difficult to add additional processing to test outputs because 1) jasmine/mocha/cucumber report test results in different ways and 2) runner doesn't expose key test outputs like which specs failed, timing, error message, etc.
NOW: jasmine/mocha/cucumber consolidate run result into a uniformed form, which runnerfork returns after it finishes running the test.