fix: remove origin config from protractorArgs if retryConfig is used #94
Conversation
Anyman552
commented
Jul 6, 2018
Anyman552
commented
- added a note that the protractor config have to be the first option in protractorArgs.
- Updated the tests for protactorRetryConfig.
- added a retryConfig example for the tests.
|
Thanks for this! I will hopefully have time to review this tomorrow and get you some feedback. |
| @@ -61,6 +61,7 @@ export default function (options = {}, callback = function noop () {}) { | |||
| } | |||
|
|
|||
| if (parsedOptions.protractorRetryConfig && retry) { | |||
| protractorArgs.splice(1, 1) | |||
There was a problem hiding this comment.
Would it make sense to actually check what is being spliced and not just blindly assume it's at index 1 ?
There was a problem hiding this comment.
That would be better. But how can we make sure it's the protractor conf? Maybe a new option named protractorConf instead put the protractor config file in protractorArgs? Then we could search for it and then remove it.
There was a problem hiding this comment.
I think having something explicit here would be good; we could require that a protractorConf option is specified alongside retryConfig; this would mean a breaking change (and major version bump) but I'm okay with that if it makes this feature work.
Users could still accidentally supply both the protractorConf option to flake and a config file in the options passed to protractor after -- but they'd run into the error in #93 so there would be some indication that something is wrong.
| @@ -214,16 +214,16 @@ describe('Protractor Flake', () => { | |||
| }) | |||
|
|
|||
| it('uses protractorRetryConfig file for spawned protractor process only after first attempt', () => { | |||
| protractorFlake({protractorRetryConfig: __dirname + '/support/protractor.flake.config.js'}) | |||
| expect(spawnStub).to.have.been.calledWith('node', [pathToProtractor(), '--params.flake.iteration', 1]) | |||
| protractorFlake({protractorArgs: [resolve(__dirname + '/support/protractor.flake.config.js')], protractorRetryConfig: resolve(__dirname + '/support/protractor.flake.retryConfig.js')}) | |||
There was a problem hiding this comment.
Since this is a unit test we could avoid creating a real file and just pass a path, what do you think?
| @@ -61,6 +61,7 @@ export default function (options = {}, callback = function noop () {}) { | |||
| } | |||
|
|
|||
| if (parsedOptions.protractorRetryConfig && retry) { | |||
| protractorArgs.splice(1, 1) | |||
There was a problem hiding this comment.
I think having something explicit here would be good; we could require that a protractorConf option is specified alongside retryConfig; this would mean a breaking change (and major version bump) but I'm okay with that if it makes this feature work.
Users could still accidentally supply both the protractorConf option to flake and a config file in the options passed to protractor after -- but they'd run into the error in #93 so there would be some indication that something is wrong.
| @@ -47,6 +47,8 @@ protractorFlake({ | |||
| nodeBin: 'node', | |||
| // set color to one of the colors available at 'chalk' - https://github.com/chalk/ansi-styles#colors | |||
| color: 'magenta', | |||
| // set the arguments for protractor | |||
| // note: the protractor config have to be the first option in protractorArgs | |||
There was a problem hiding this comment.
What about putting this note on retryConfig instead of protractorConfig because this is only an issue when using retryConfig?
Also minor grammar note, I think the sentence would be better reworded as "Arguments passed to protrractor. The path to the protractor config must be the first argument."
There was a problem hiding this comment.
Yes that would be better. If we specify a protractorConf option, this note will be outdated.
