Skip to content

fix: remove origin config from protractorArgs if retryConfig is used #94

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: remove origin config from protractorArgs if retryConfig is used #94

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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."

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that would be better. If we specify a protractorConf option, this note will be outdated.

protractorArgs: [],
// specify a different protractor config to apply after the first execution attempt
// either specify a config file, or cli args (ex. --capabilities.browser=chrome)
Expand Down
1 change: 1 addition & 0 deletions src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ export default function (options = {}, callback = function noop () {}) {
}

if (parsedOptions.protractorRetryConfig && retry) {
protractorArgs.splice(1, 1)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to actually check what is being spliced and not just blindly assume it's at index 1 ?

Copy link
Author

@Anyman552 Anyman552 Jul 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

protractorArgs.push(parsedOptions.protractorRetryConfig)
}

Expand Down
10 changes: 5 additions & 5 deletions test/unit/index.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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')})
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a unit test we could avoid creating a real file and just pass a path, what do you think?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right.

expect(spawnStub).to.have.been.calledWith('node', [pathToProtractor(), resolve(__dirname + '/support/protractor.flake.config.js'), '--params.flake.iteration', 1])
spawnStub.dataCallback(failedSingleTestOutput)
spawnStub.endCallback(1)
expect(spawnStub).to.have.been.calledWith('node', [pathToProtractor(), '--params.flake.iteration', 2, '--params.flake.retry', true, '--specs', '/tests/a-flakey.test.js', resolve(__dirname + '/support/protractor.flake.config.js')])
expect(spawnStub).to.have.been.calledWith('node', [pathToProtractor(), '--params.flake.iteration', 2, '--params.flake.retry', true, '--specs', '/tests/a-flakey.test.js', resolve(__dirname + '/support/protractor.flake.retryConfig.js')])
})

it('uses protractorRetryConfig cli args for spawned protractor process only after first attempt', () => {
protractorFlake({protractorRetryConfig: '--capabilities.browser=chrome --capabilities.sharding=false'})
expect(spawnStub).to.have.been.calledWith('node', [pathToProtractor(), '--params.flake.iteration', 1])
protractorFlake({protractorArgs: [resolve(__dirname + '/support/protractor.flake.config.js')], protractorRetryConfig: '--capabilities.browser=chrome --capabilities.sharding=false'})
expect(spawnStub).to.have.been.calledWith('node', [pathToProtractor(), resolve(__dirname + '/support/protractor.flake.config.js'), '--params.flake.iteration', 1])
spawnStub.dataCallback(failedSingleTestOutput)
spawnStub.endCallback(1)
expect(spawnStub).to.have.been.calledWith('node', [pathToProtractor(), '--params.flake.iteration', 2, '--params.flake.retry', true, '--specs', '/tests/a-flakey.test.js', '--capabilities.browser=chrome --capabilities.sharding=false'])
Expand Down
20 changes: 20 additions & 0 deletions test/unit/support/protractor.flake.retryConfig.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// An example configuration file
exports.config = {
// The address of a running selenium server.
seleniumAddress: 'http://localhost:4444/wd/hub',

// Capabilities to be passed to the webdriver instance.
capabilities: {
browserName: 'chrome'
},

// Spec patterns are relative to the configuration file location passed
// to protractor (in this example conf.js).
// They may include glob patterns.
specs: ['example-spec.js'],

// Options to be passed to Jasmine-node.
jasmineNodeOpts: {
showColors: true // Use colors in the command line report.
}
}