-
Notifications
You must be signed in to change notification settings - Fork 172
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
Added TestAndWait for CLI & runTestAndWait for Script #155
Conversation
lib/webpagetest.js
Outdated
@@ -531,199 +537,15 @@ function runTest(what, options, callback) { | |||
} | |||
|
|||
function runTestAndWait(what, options, callback) { | |||
delete options.pollResults; | |||
delete options.pollResults, options.timeout; | |||
delete options.timeout; |
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.
This appears to be a duplicate line.
lib/webpagetest.js
Outdated
@@ -531,199 +537,15 @@ function runTest(what, options, callback) { | |||
} | |||
|
|||
function runTestAndWait(what, options, callback) { | |||
delete options.pollResults; | |||
delete options.pollResults, options.timeout; |
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.
Hmm...do we want to delete timeout? Like...if they pass a timeout, then runTest would use that and it lets them still put a time limit on runTestAndWait, right? Seems handy to keep it around?
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 sorry left that one unknowingly, Cool I will let users to pass the timeout. Well at first I thought if we are creating a dedicated function for run test and wait then it should totally wait 🙂.
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.
Yeah...I do like us not requiring timeout here at all, but I think it's nice to be able to override if necessary.
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.
Super minor stuff...looking great!
Added testAndWait (CLI) & runTestAndWait method for Wrapper. A bit of a hacky way and can definitely be improved in future. It basically runs the test and waits for the whole execution process (Has an inbuilt pollResults param whether a user provides it or not and deletes the timeout param if provided by the user), then returns the full data object