-
Notifications
You must be signed in to change notification settings - Fork 672
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
Implement CLI (closes #105) #118
Conversation
} | ||
|
||
function waitBrowserConnectionReady (browserConnection) { | ||
return new Promise((resolve)=> browserConnection.once('ready', resolve)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(resolve)
-> resolve
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch
I'm executing
|
/r- |
In addition: Hammerhead errors are not handled correctly |
this.opts = this.program.opts(); | ||
|
||
// NOTE: '-list-browsers' option just lists browsers and immediately exits app. | ||
// Therefore, we don't need process other args |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NOTE: the '-list-browsers' option only lists browsers and immediately exits the app.
Therefore, we don't need to process other arguments.
\r- |
.catch(done); | ||
}); | ||
|
||
it("Should parse screenshots path and ensure it's existence", function (done) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one more - it's
-> its
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or even better
Should parse the screenshot path and ensure it exists
\r- |
@AlexanderMoskovkin Definitely no. Need to figure out optimal string lengths with the final texts (which we now have thanks to @VasilyStrelyaev) |
@@ -42,7 +42,7 @@ | |||
CURRENT_TEST_STEP_NAME: nextStep ? stepNames[nextStep - 1] : 'Test initialization', | |||
BROWSER_STATUS_URL: '{{{browserStatusUrl}}}', | |||
TAKE_SCREENSHOT_ON_FAILS: {{{takeScreenshotOnFails}}}, | |||
FAIL_ON_JS_ERRORS: {{{failOnJsErrors}}}, | |||
SKIP_JS_ERRORS: {{{failOnJsErrors}}}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{{{failOnJsErrors}}}
should be changed with {{{skipJsErrors}}}
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes!
/r- |
FPR |
lgtm it works nice on my side |
lgtm |
ping @churkin |
lgtm |
Waiting for @VasilyStrelyaev |
.option('-b, --list-browsers', 'output the available browser aliases') | ||
.option('-r, --reporter <name>', 'specify the reporter type to use') | ||
.option('-p, --report-path <path>', 'specify the path to which the report file should be saved') | ||
.option('-s, --screenshots <path>', 'enable screenshot capturing and specify the path to save them to') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops... 'them' doesn't relate to anything anymore...
enable screenshot capturing and specify the path to save the screenshots to
/r- |
lgtm |
Implement CLI (closes #105)
You can try it locally (please try it) by running in the shell:
Then to run CLI:
\cc @churkin @AlexanderMoskovkin @kirovboris
@VasilyStrelyaev check texts and test names, please