Reject when main request fails #1174

Janpot · 2016-12-17T22:30:31Z

Tests might need some reordering

ebidel · 2016-12-30T18:56:39Z

lighthouse-core/gather/gather-runner.js

@@ -203,6 +203,10 @@ class GatherRunner {
      log.log('status', status);
      return driver.endNetworkCollect();
    }).then(networkRecords => {
+      if (driver.online && networkRecords[0].failed) {
+        log.error('statusEnd', networkRecords[0].localizedFailDescription);
+        return Promise.reject(new Error(networkRecords[0].localizedFailDescription));


How about something more friendly for the message?

`${options.url} could not be resolved`

The request could've failed for other reasons than 'not resolved'. Maybe my PR title is a bit too specific.

ebidel · 2016-12-30T18:58:39Z

lighthouse-core/test/runner-test.js

+      online: false,
+      endNetworkCollect() {
+        return Promise.resolve(
+          recordsFromLogs(require('./fixtures/unresolved-perflog.json'))


might as well require this once, at the top of the file.

ebidel · 2016-12-30T19:01:37Z

lighthouse-core/test/runner-test.js

+      .then(_ => {
+        assert.ok(false);
+      }, error => {
+        assert.ok(true);


this check doesn't do much.

what do you mean? The result throws when run resolves and resolves when run throws. What do you suggest instead?

I just meant that you're checking if you get here, the promise rejects. But there's another assert.equal that falls under this rejection too. I think it's fine too keep as you have. Doesn't hurt.

ebidel · 2016-12-30T19:03:51Z

lighthouse-core/test/runner-test.js

+    return Runner.run({}, {url, config, driverMock: unresolvedDriver})
+      .then(_ => {
+        assert.ok(true);
+      });


probably should have a reject case here with assert.ok(false);

Why? Mocha handles rejected promises just fine.

To be consistent with 151-153 and make sure your code isn't throwing.

ebidel · 2016-12-30T19:07:38Z

Nice change. I've been bitten by a faulty url a couple of times when LH tests the offline page in Chrome :)

Is there a way to exit cleanly in this case without causing a runtime error? Similar to what LH does when you do not provide a URL. Although catching the error is better than we had before, this is fairly unfriendly:

paulirish · 2017-01-04T03:52:40Z

@ebidel use yarn run instead of npm run to make the bottom of that screenshot not terrible. :)

ebidel · 2017-01-04T04:39:54Z

This works for me.

ebidel · 2017-01-04T04:40:09Z

Leaving it open for anyone else to add comments.

brendankenny

looking good.

This error will also need to be handled in the extension here so we don't get a bunch of issues filed for testing non existent pages :)

brendankenny · 2017-01-05T23:16:42Z

lighthouse-core/test/runner-test.js

@@ -126,6 +128,61 @@ describe('Runner', () => {
      });
  });

+  it('rejects when domain name can\'t be resolved', () => {


It would be great if these could be narrowed in scope by adding them to gather-runner-test.js instead and cutting out Runner from the tests

brendankenny · 2017-01-05T23:24:05Z

lighthouse-cli/bin.ts

@@ -241,8 +242,15 @@ function showProtocolTimeoutError() {
  process.exit(_PROTOCOL_TIMEOUT_EXIT_CODE);
 }

+function showPageLoadError() {
+  console.error('Unable to load the page.');
+  process.exit(_PAGE_LOAD_ERROR_EXIT_CODE);


interesting. Any reason to have a special error code (vs using _RUNTIME_ERROR_CODE)? No big deal if so, we added _PROTOCOL_TIMEOUT_EXIT_CODE specifically because we needed to detect that case for Travis retries

nope, no reason. Just trying to match the style of the surrounding code. Will change to _RUNTIME_ERROR_CODE

brendankenny · 2017-01-05T23:25:04Z

lighthouse-cli/bin.ts

@@ -241,8 +242,15 @@ function showProtocolTimeoutError() {
  process.exit(_PROTOCOL_TIMEOUT_EXIT_CODE);
 }

+function showPageLoadError() {
+  console.error('Unable to load the page.');


maybe add something friendly here about rechecking the URL and that the page is actually working?

brendankenny · 2017-01-05T23:33:21Z

lighthouse-core/gather/gather-runner.js

@@ -203,6 +203,12 @@ class GatherRunner {
      log.log('status', status);
      return driver.endNetworkCollect();
    }).then(networkRecords => {
+      if (driver.online && networkRecords[0].failed) {
+        log.error('statusEnd', networkRecords[0].localizedFailDescription);


log.error('GatherRunner', networkRecords[0].localizedFailDescription);

Janpot · 2017-01-07T13:57:17Z

@brendankenny Can you take another look?

ebidel · 2017-01-10T19:29:12Z

@Janpot can you look into the failing tests?

Janpot · 2017-01-11T08:32:29Z

@ebidel Ok, so I can't reproduce this locally but here's what's happening on travis for some reason:

It fails on http://localhost:10200/dobetterweb/dbw_tester.html in smokehouse:

apparently networkRecords are not in order. On travis I get a first entry networkRecords[0] for 'http://localhost:10200/dobetterweb/dbw_partial_a.html' Instead of 'http://localhost:10200/dobetterweb/dbw_tester.html'. This entry fails because for some reason it is blocked by csp (_blockedReason: 'csp'). I'm not sure why this is happening but maybe it is something you want to look into as it seems an unexpected side effect of the dbw test?

In the meantime I will fix the logic by selecting the correct resource with the url. Will have to refactor a few things as there's a lot of mocking in the tests that breaks when I add my solution. Will continue on this when I have time again.

Janpot · 2017-01-11T21:30:19Z

@ebidel Ok, seems to work now.

In case you want to check this, here's the strange line. This is a log of networkRecords[0] in afterPass. Notice it's not the main document and also the csp error. This doesn't happen on my machine.

brendankenny · 2017-01-11T22:38:49Z

lighthouse-extension/app/src/popup.js

@@ -172,6 +172,11 @@ document.addEventListener('DOMContentLoaded', _ => {
        }
      }

+      if (err.code === 'PAGE_LOAD_ERROR') {


sorry to add one last thing, but it would be great to fold this into the known error messages handling above. Error matching is on message string, not code, but since it appears we never actually expose the message directly, it would be easy to prefix mainRecord.localizedFailDescription with some string ('Unable to load the page: ' or whatever), then use that as the key in NON_BUG_ERROR_MESSAGES

brendankenny · 2017-01-11T22:39:46Z

nice catch on network record ordering. Beyond the DBW smoke test CSP issue, it's nice to have the more robust search for the root network record in this PR

Janpot · 2017-01-11T22:58:41Z

Another thing I noticed (but wasn't really an issue) was that chrome on Travis is out of date. Might want to clear the cache there once.

brendankenny

☁️ ➡️ ✂️ ➡️ 💻 🚫
LGTM!

Janpot · 2017-01-11T23:38:32Z

@brendankenny Your emoji's... Does it mean you're manually squashing and merging to master? If so, I'll make sure to squash and rebase in the future to save you the hassle (I usually use the GitHub squash+rebase)

brendankenny · 2017-01-11T23:40:56Z

no, squashed via github UI

☁️ ➡️ ✂️ ➡️ 💻 🚫

Obviously this is a pictographic representation of displaying an error when the computer's connection to the test page (in the cloud) is severed :P

Reject when domain name can’t be resolved

7f8ba87

ebidel suggested changes Dec 30, 2016

View reviewed changes

Janpot changed the title ~~Reject when domain name can’t be resolved~~ Reject when main request fails Dec 31, 2016

Address some comments

3f4a399

ebidel approved these changes Jan 4, 2017

View reviewed changes

paulirish requested a review from brendankenny January 5, 2017 09:30

brendankenny requested changes Jan 5, 2017

View reviewed changes

Janpot added 4 commits January 7, 2017 14:45

Address more comments

374df52

Merge remote-tracking branch 'upstream/master' into dns-error

5d54cb6

Anticipate error in extension

3d4dab0

Can go on one line now

5986667

Janpot added 2 commits January 10, 2017 12:18

Make linter happy

9877a74

Strange, those errors didn’t get picked up locally

069945e

Janpot added 7 commits January 10, 2017 23:40

Temporarily set lighthouse to verbose

9cedc29

log network entry

b5d105c

print chrome version

5fdbe00

try and update chrome

ba03ebf

ignore prompt

2069baa

first record can fail

85473f2

d’oh, lint…

b9077ad

Janpot added 3 commits January 11, 2017 21:38

Fix mocks

4b6f9f4

set logging

4ef9354

Turns out it was just flaky?

757faaa

brendankenny reviewed Jan 11, 2017

View reviewed changes

Use existing ignore system

ef27297

brendankenny approved these changes Jan 11, 2017

View reviewed changes

brendankenny merged commit 3e1ddb6 into GoogleChrome:master Jan 11, 2017

Janpot deleted the dns-error branch January 11, 2017 23:23

andrewrota pushed a commit to andrewrota/lighthouse that referenced this pull request Jan 13, 2017

Stop run if main request fails (GoogleChrome#1174)

9aa115c

patrickhulce mentioned this pull request Jun 8, 2017

fix(http-redirect): do not error if no server listens on http #2461

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reject when main request fails #1174

Reject when main request fails #1174

Janpot commented Dec 17, 2016

ebidel Dec 30, 2016

Janpot Dec 31, 2016

ebidel Dec 30, 2016

ebidel Dec 30, 2016

Janpot Dec 31, 2016

ebidel Dec 31, 2016

ebidel Dec 30, 2016

Janpot Dec 31, 2016

ebidel Dec 31, 2016

ebidel commented Dec 30, 2016

paulirish commented Jan 4, 2017

ebidel commented Jan 4, 2017

ebidel commented Jan 4, 2017

brendankenny left a comment

brendankenny Jan 5, 2017

brendankenny Jan 5, 2017

Janpot Jan 7, 2017

brendankenny Jan 5, 2017

brendankenny Jan 5, 2017

Janpot commented Jan 7, 2017

ebidel commented Jan 10, 2017

Janpot commented Jan 11, 2017

Janpot commented Jan 11, 2017

brendankenny Jan 11, 2017

brendankenny commented Jan 11, 2017

Janpot commented Jan 11, 2017

brendankenny left a comment

Janpot commented Jan 11, 2017

brendankenny commented Jan 11, 2017

Reject when main request fails #1174

Reject when main request fails #1174

Conversation

Janpot commented Dec 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebidel commented Dec 30, 2016

paulirish commented Jan 4, 2017

ebidel commented Jan 4, 2017

ebidel commented Jan 4, 2017

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Janpot commented Jan 7, 2017

ebidel commented Jan 10, 2017

Janpot commented Jan 11, 2017

Janpot commented Jan 11, 2017

Choose a reason for hiding this comment

brendankenny commented Jan 11, 2017

Janpot commented Jan 11, 2017

brendankenny left a comment

Choose a reason for hiding this comment

Janpot commented Jan 11, 2017

brendankenny commented Jan 11, 2017