chore: rework disposers into a frame task #1413
Conversation
pavelfeldman
force-pushed
the
frame_task
branch
from
73c57ba to
e933204
Compare
| @@ -897,7 +897,6 @@ module.exports.describe = function({testRunner, expect, playwright, MAC, WIN, FF | |||
| await page.$eval('iframe', frame => frame.remove()); | |||
| const error = await navigationPromise; | |||
| expect(error.message).toContain('frame was detached'); | |||
| expect(error.stack).toContain('Frame.goto') | |||
There was a problem hiding this comment.
this was very important for debugging.
There was a problem hiding this comment.
I looked at the stacks without it and it does look very straightforward. Could you give an example that was good and is now puzzling?
There was a problem hiding this comment.
It's not as bad as it once was, because the async stack traces help us at least get the client code that called this. But lacking the playwright method you called in the stack trace is not great.
| this.done(); | ||
| if (this._url) | ||
| error.message = error.message + ` while navigating to ${this._url}`; | ||
| throw error; |
There was a problem hiding this comment.
Never throw someone else's error to avoid hiding your stack trace.
There was a problem hiding this comment.
I'm not avoiding, but I don't think stack matters much here. I don't want to instanceof TimeoutError and rewrite its message - that sounds lame...
|
|
||
| if (!error) | ||
| return result!; | ||
| this.done(); |
There was a problem hiding this comment.
Calling done() at the right time is a bit tricky. Returning error was easier in that regard.
|
Seems like a marginal improvement. If we were to somehow simplify |
No description provided.