Skip to content
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.

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

command-line parameters for retry attempts fix #67

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

allbigbopper
Copy link

@allbigbopper allbigbopper commented Nov 6, 2019

There is a fix for this issue [https://github.com//issues/64](Can't handle command-line parameters and using data from protractor.conf.js file for retry attempts instead). Would really appreciate if you take a look into it @davglass . Thanks!

@allbigbopper
Copy link
Author

Hi @davglass, @amrot17. Could you please look into this one?

@amilbeck
Copy link

amilbeck commented Nov 18, 2019

Hey @allbigbopper, I just tried this out with a small test suite from my setup and it didn't work. I turned on the debug statements so I could see the command that was actually running. There is a slight difference between the command between runs 1 and 2.

The first time the command is run the object looks like this:

ProtractorRetry ProtractorCommand {
    _: [ './dist/e2e/config/protractor-conf.js' ],
    help: false,
    version: false,
    baseUrl: 'https://my-url.com',
    params: { env: 'release' },
    suite: 'my_failing_suite',
    '$0': 'node_modules\\protractor\\bin\\protractor'
}

With the changes from this PR, the second attempt looks like it sent the command over as an array instead of a JSON object.

ProtractorRetry Command node_modules\protractor\bin\protractor [
  [ './dist/e2e/config/protractor-conf.js' ],
  '--specs',
  'C:\\repos\\my-app\\dist\\e2e\\specs\\my-failing-spec.spec.js',
  '--retry',
  1,
  '--disableChecks',
  '--baseUrl',
  'https://my-url.com',
  '--params.env',
  'release'
] 

It looks like it picked up all the command line flags I started the first run with but the command object is just not in the correct format. It's also missing the '$0' property. (Not sure if that actually matters). In any case it looks like this one is on the right path but just not quite there yet.

EDIT: Actually, I take that back. It looks like it does work. I just needed to change the way I am processing --params.env on my end and it started working.

@allbigbopper
Copy link
Author

allbigbopper commented Nov 18, 2019

Thanks for review, really appreciate! Maybe I was just confused with 'var protractorCommand = []', and all .push() methods so it looked for me like an array, so I implemented it as an array and when I tested it perfectly worked for my project. Could you please share your small test project so I could check why it doesn't work?

@amilbeck
Copy link

See my edit to my previous comment. It was an issue on my end that was causing it to fail. Once I updated it started working.

@allbigbopper
Copy link
Author

oh, it's great to hear 👍

@allbigbopper
Copy link
Author

Hi @amilbeck. Do you think we could merge it?

@dreuxl
Copy link
Contributor

dreuxl commented Nov 25, 2019

sorry for delay, I maybe able to take a look at this one this week.

@viktorgogulenko
Copy link

Hey, guys. I'm really waiting for the fix of this issue as I have to generate lot of conf-files for every change instead of cli parameters. Thanks in advance for fix!

@amilbeck
Copy link

Hi @amilbeck. Do you think we could merge it?

I'm not a maintainer of this repo but for what it's worth, I forked this repo and included your changes. I've just been using my forked repo and it's been working fine for me for the last couple weeks. My guess is it's probably fine to merge but I don't have the final say.

@allbigbopper
Copy link
Author

Hi @dreuxl! Do you think we could merge it?

@allbigbopper
Copy link
Author

Hi @dreuxl! Could we please merge these changes?

@allbigbopper
Copy link
Author

Hi @amilbeck, do you think we could merge it? We really need this fix. A fix has been around for 2 month

@amilbeck
Copy link

I think so. It's been working fine for me on my forked repo. I haven't had any issues at all. Been using it since Nov. 18, 2019. Works on both Linux and Windows for me. My local dev env is Windows and CICD pipeline runs on Linux. It's working great in both places.

@allbigbopper
Copy link
Author

@dreuxl could you merge it please?? we really need a fix

@dreuxl dreuxl mentioned this pull request Mar 6, 2020
@dreuxl
Copy link
Contributor

dreuxl commented Mar 11, 2020

ok I had to do my own PR for that because the test steps are not working for external PR , due to travis credential restriction to repo owner.

let me know how it goes @allbigbopper

@viktorgogulenko
Copy link

Any updates?

@dreuxl
Copy link
Contributor

dreuxl commented Mar 25, 2020

fixed in PR #70

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants