chore(promises): Wait for promises explicitly #70
Conversation
sjelin
force-pushed
the
promise
branch
3 times, most recently
from
d45aa6e to
4ab4f2f
Compare
| } | ||
| }, flow); | ||
| }, 'Run ' + fnName + description + ' in control flow').then(seal(done), function(err) { |
There was a problem hiding this comment.
The seal function isn't used any more, so its declaration can be removed.
| @@ -78,30 +94,38 @@ function wrapInControlFlow(flow, globalFn, fnName) { | |||
|
|
|||
| flow.execute(function controlFlowExecute() { | |||
| return new webdriver.promise.Promise(function(fulfill, reject) { | |||
| function wrappedReject(err) { | |||
There was a problem hiding this comment.
nit: inconsistent indentation
| } | ||
|
|
||
|
|
||
| /** | ||
| * Wraps a function so it runs inside a webdriver.promise.ControlFlow and | ||
| * waits for the flow to complete before continuing. | ||
| * @param {!Function} globalFn The function to wrap. |
There was a problem hiding this comment.
not related to this PR: only one param of the 3 is documented
| it('test B', () => { | ||
| currentTest = 'B'; | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Would be nice to test the error handling code paths somehow too.
sjelin
force-pushed
the
promise
branch
3 times, most recently
from
4b57ea0 to
c67de4a
Compare
| @@ -102,4 +102,24 @@ describe('things that should fail', function() { | |||
| expect(fakeDriver.getDecimalNumber()).toBeCloseTo(3.1); | |||
| expect(fakeDriver.getDecimalNumber()).not.toBeCloseTo(3.14); | |||
| }); | |||
|
|
|||
| describe('native promises', function() { | |||
There was a problem hiding this comment.
@thorn0 this look good for error handling tests?
There was a problem hiding this comment.
Yes, thanks. There is also all this code for dealing with stack traces though...
There was a problem hiding this comment.
You want me to double check that the stack trace is correct?
There was a problem hiding this comment.
Never mind. That code appeared to me more complicated than it actually is. Let's merge this. 👍
| err.stack = ''; | ||
| } | ||
| err.stack = err.stack + '\nFrom asynchronous test: \n' + driverError.stack; | ||
| callWhenIdle(flow, done.fail.bind(done, err)); |
There was a problem hiding this comment.
We might want to call flow.reset() here instead of waiting for idle. What do you think?
There was a problem hiding this comment.
Why would we want to reset? If there are pending webdriver tasks we probably want to wait on them
There was a problem hiding this comment.
If we know that the test has already failed and we can cancel those tasks, why execute them? But I'm not sure. Let's leave it as it is.
| * @param {!Function} globalFn The function to wrap. | ||
| * @parm {!string} fnName The name of the function being wrapped (e.g. `'it'`) |
There was a problem hiding this comment.
- Nit: address spelling.
- Nit: should end comment with a period.
| err = new Error('Unknown Error'); | ||
| err.stack = ''; | ||
| } | ||
| err.stack = err.stack + '\nFrom asynchronous test: \n' + driverError.stack; |
There was a problem hiding this comment.
Why not add fnName and description to this information for it to be a bit easier to identify errors?
err.stack += '\nFrom asynchronous test ' + fnName + description + ': \n' + driverError.stack;There was a problem hiding this comment.
jasmine adds that information automatically when we call done.fail
|
@cnishina you wanna review this? |
| * Wraps a function so it runs inside a webdriver.promise.ControlFlow and | ||
| * waits for the flow to complete before continuing. | ||
| * @param {!webdriver.promise.ControlFlow} flow The WebDriver control flow |
There was a problem hiding this comment.
Nit: should end comment with a period.
| * @param {!Function} globalFn The function to wrap. | ||
| * @parm {!string} fnName The name of the function being wrapped (e.g. `'it'`) |
There was a problem hiding this comment.
- Nit: address spelling.
- Nit: should end comment with a period.
See angular#68 for details
|
@cnishina all comments addressed. Can I have an LGTM? 😄 |
See #68 for details