fix: failed loading message #1603
Conversation
| log.error('GatherRunner', mainRecord.localizedFailDescription); | ||
| const error = new Error(`Unable to load the page: ${mainRecord.localizedFailDescription}`); | ||
| if (driver.online && (!mainRecord || mainRecord.failed)) { | ||
| const failDescription = mainRecord ? |
There was a problem hiding this comment.
nit: this can probably fit on one line.
There was a problem hiding this comment.
not quite, should I make the name shorter?
There was a problem hiding this comment.
fitting would be nice, but not a big deal.
|
Ironically, tests are failing b/c of shop. |
But isn't the error message you get so much friendlier? 😄 |
|
will do 👍 |
|
PTAL |
| @@ -125,6 +125,22 @@ class GatherRunner { | |||
| } | |||
|
|
|||
| /** | |||
| * @param {string} url | |||
| * @param {{online: boolean}} driver | |||
| * @param {!Array<WebInspector.NetworkRequest} networkRecords | |||
| @@ -125,6 +125,22 @@ class GatherRunner { | |||
| } | |||
|
|
|||
| /** | |||
| * @param {string} url | |||
There was a problem hiding this comment.
single line about what it's doing?
| GatherRunner.assertPageLoaded(url, {online: true}, records); | ||
| }, /Unable.*timeout/); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
worth moving rejects when domain name can\'t be resolved and resolves when domain name can\'t be resolved but is offline tests into this describe block? They do test the whole pipeline, not just the function, but they're more or less just unit tests of the function...
There was a problem hiding this comment.
I've noticed we have some differing philosophies when it comes to describe blocks and test organization in general, haha, so I'm opposed but will defer to you if you want 'em moved :)
There was a problem hiding this comment.
I have no coherent philosophy when it comes to organization of tests into sub-blocks so keeping as-is is good :)
|
☁️ |
fixes #1602