report(redirects): reformat results, incl all requests and wasted time, #3492

paulirish · 2017-10-06T00:05:07Z

Changes:

The cost of each redirect is shown next to the new/redirected URL rather than initial URL. (Makes more sense to my brain). This also means we now show the final URL in the table, whereas before it wasn't there.
The very first initial url is shown for completeness.
(If there's just 1 redirect we currently pass them. Nothing changes here.)
All costs of redirects are kept and summed. We don't skip the first one.
Even if there's 1 redirect and you pass, we will still show the cost of the redirects to be clear.
I updated the test cases to be slightly more realistic.

screenshots

failing with 2 redirects:

passing with 1 redirect:

passing with 0 redirects:

paulirish · 2017-10-06T00:06:02Z

lighthouse-core/audits/redirects.js

+
+          // Only create a table (with the initial request) if there are > 1 redirects
+          if (!initialRequest) {
+            if (redirectRequests.length > 1) {


This >1 and a === 2 below could instead use a ACCEPTED_REDIRECTS const if folks prefer.

wardpeet · 2017-10-06T05:49:54Z

lighthouse-core/test/audits/redirects-test.js

  startTime: 17,
+  url: 'http://exampel.com/',


typo example.com?

this was deliberate. :)

wardpeet

Looks good, thanks for cleaning up! Really appreciate it! 🥇
Just a few remarks about urls and spelling 😄

wardpeet · 2017-10-06T05:51:36Z

lighthouse-core/test/audits/redirects-test.js

      startTime: 12,
      url: 'https://m.example.com/',
    },
  ],
 };

+const FAILING_TWO_REDIRECTS = {
+  startTime: 446.286,
+  url: 'http://lisairish.com/',


perhaps better to just use example.com as an url so it's consistent with the rest?

Or is this a marketing stunt for your mom's website? 😆

Well i tested this with these actual URLs, which have this set of redirects setup.

patrickhulce · 2017-10-06T16:07:18Z

lighthouse-core/audits/redirects.js

          {key: 'wastedMs', itemType: 'text', text: 'Time for Redirect'},
        ];
        const details = Audit.makeTableDetails(headings, pageRedirects);

        return {
-          score: UnusedBytes.scoreForWastedMs(totalWastedMs),
+          // We award a passing grade if you only have 1 redirect
+          score: redirectRequests.length === 2 ? 100 : UnusedBytes.scoreForWastedMs(totalWastedMs),


if totalWastedMs === 0 scoreForWastedMs doesn't already give you a 100?

With the new logic you can get totalWastedMs > 0 and still get a passing grade. Take http://www.lisairish.com which i was seeing it take 500ms to redirect to https://www.lisairish.com. We should still tell them about the 500ms even if we pass them.

could we add a passing test case for a resource with 0 redirects too? I had to pause for a second on this, alternatively switch to redirectRequests.length <= 2

already have passing test case for 0 redirects.
changed to <= 2.

brendankenny

need to update the smoke test expectations for the additional URL in the details list. Not sure about the other failure though...was that expected?

brendankenny · 2017-10-06T19:13:18Z

was that expected?

ah, yes, it was:

All costs of redirects are kept and summed. We don't skip the first one.

brendankenny · 2017-10-06T20:27:59Z

lighthouse-core/audits/redirects.js

-          // We allow 1 redirect (www. => m.)
-          if (i > 1) {
-            totalWastedMs += wastedMs;
+        for (let i = 0; i < redirectRequests.length; i++) {


the if (!initialRequest) part of this ends up being a little confusing. What about pulling it out, something like

let totalWastedMs = 0; const pageRedirects = []; if (redirectRequests.length > 1) { pageRedirects.push({ url: `(Initial: ${redirectRequests[0].url})`, wastedMs: 'n/a', }); } for (let i = 1; i < redirectRequests.length; i++) { const initialRequest = redirectRequests[i - 1]; const redirectedRequest = redirectRequests[i]; const wastedMs = (redirectedRequest.startTime - initialRequest.startTime) * 1000; totalWastedMs += wastedMs; pageRedirects.push({ url: redirectedRequest.url, wastedMs: Util.formatMilliseconds(wastedMs, 1), }); }

(I believe that should be equivalent)

paulirish · 2017-10-09T22:57:15Z

updated! @brendankenny ptal.

brendankenny · 2017-10-10T17:31:10Z

will you rebase to pick up the passive event listener fix?

brendankenny

ptal :)

paulirish · 2017-10-20T00:36:43Z

@brendankenny thar u go

brendankenny

LGTM, sorry I missed your rebase!

brendankenny · 2017-10-30T21:17:04Z

@patrickhulce if you have any last issues

patrickhulce · 2017-10-30T21:23:25Z

lighthouse-core/test/audits/redirects-test.js

+      assert.equal(output.details.items.length, 4);
+      assert.equal(output.rawValue, 17000);
+    });
+  });


nit: add newline

patrickhulce

lgtm % nits

patrickhulce · 2017-10-30T21:25:01Z

lighthouse-core/audits/redirects.js

          {key: 'wastedMs', itemType: 'text', text: 'Time for Redirect'},
        ];
        const details = Audit.makeTableDetails(headings, pageRedirects);

        return {
-          score: UnusedBytes.scoreForWastedMs(totalWastedMs),
+          // We award a passing grade if you only have 1 redirect
+          score: redirectRequests.length === 2 ? 100 : UnusedBytes.scoreForWastedMs(totalWastedMs),


could we add a passing test case for a resource with 0 redirects too? I had to pause for a second on this, alternatively switch to redirectRequests.length <= 2

…e, (GoogleChrome#3492)

paulirish requested a review from wardpeet October 6, 2017 00:05

paulirish commented Oct 6, 2017

View reviewed changes

wardpeet reviewed Oct 6, 2017

View reviewed changes

wardpeet approved these changes Oct 6, 2017

View reviewed changes

patrickhulce reviewed Oct 6, 2017

View reviewed changes

brendankenny reviewed Oct 6, 2017

View reviewed changes

paulirish changed the title ~~Redirects audit: formatting~~ report(redirects): reformat results, including all requests and wasted time Oct 9, 2017

paulirish changed the title ~~report(redirects): reformat results, including all requests and wasted time~~ report(redirects): reformat results, incl all requests and wasted time Oct 9, 2017

paulirish modified the milestones: Sprint Uno: Oct 2-13, Sprint Dos: Oct 16-27 Oct 16, 2017

paulirish added the waiting4committer label Oct 17, 2017

brendankenny requested changes Oct 20, 2017

View reviewed changes

paulirish force-pushed the redirects-hax branch from 714bbad to 0b032a9 Compare October 20, 2017 00:33

paulirish added 3 commits October 19, 2017 17:35

Redirects: formatting

5e3b6a8

extract conditional. thx brendan

dcb24df

rebaseline smoke test.

0b032a9

paulirish added waiting4reviewer and removed waiting4committer labels Oct 20, 2017

paulirish changed the title ~~report(redirects): reformat results, incl all requests and wasted time~~ report(redirects) reformat results, incl all requests and wasted time Oct 26, 2017

paulirish changed the title ~~report(redirects) reformat results, incl all requests and wasted time~~ report(redirects): reformat results, incl all requests and wasted time Oct 26, 2017

paulirish changed the title ~~report(redirects): reformat results, incl all requests and wasted time~~ report(redirects): reformat results, incl all requests and wasted time. Oct 26, 2017

paulirish changed the title ~~report(redirects): reformat results, incl all requests and wasted time.~~ report(redirects): reformat results, incl all requests and wasted time Oct 26, 2017

paulirish changed the title ~~report(redirects): reformat results, incl all requests and wasted time~~ report(redirects): reformat results, incl all requests and wasted time, Oct 26, 2017

paulirish changed the title ~~report(redirects): reformat results, incl all requests and wasted time,~~ report(redirects): reformat results, incl all requests and wasted time,. Oct 26, 2017

paulirish changed the title ~~report(redirects): reformat results, incl all requests and wasted time,.~~ report(redirects): reformat results, incl all requests and wasted time, Oct 26, 2017

paulirish changed the title ~~report(redirects): reformat results, incl all requests and wasted time,~~ report(redirects): reformat results, incl all requests and wasted time,. Oct 26, 2017

paulirish changed the title ~~report(redirects): reformat results, incl all requests and wasted time,.~~ report(redirects): reformat results, incl all requests and wasted time, Oct 26, 2017

paulirish modified the milestones: Sprint Dos: Oct 16-27, Sprint Tres: Oct 30 - Nov 12 Oct 30, 2017

brendankenny approved these changes Oct 30, 2017

View reviewed changes

brendankenny assigned patrickhulce Oct 30, 2017

patrickhulce reviewed Oct 30, 2017

View reviewed changes

patrickhulce approved these changes Oct 30, 2017

View reviewed changes

patrickhulce added waiting4committer and removed waiting4reviewer labels Oct 30, 2017

paulirish merged commit fbe3fda into master Oct 31, 2017

paulirish deleted the redirects-hax branch October 31, 2017 23:31

logic <=

e50ffc4

christhompson pushed a commit to christhompson/lighthouse that referenced this pull request Nov 28, 2017

report(redirects): reformat results, incl all requests and wasted tim…

d71b189

…e, (GoogleChrome#3492)

dependencies bot mentioned this pull request Dec 17, 2017

Update lighthouse in / from 2.5.0 to 2.7.0 chauncey-garrett/dotfiles#57

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

report(redirects): reformat results, incl all requests and wasted time, #3492

report(redirects): reformat results, incl all requests and wasted time, #3492

paulirish commented Oct 6, 2017 •

edited

Loading

paulirish Oct 6, 2017

wardpeet Oct 6, 2017 •

edited

Loading

paulirish Oct 9, 2017

wardpeet left a comment

wardpeet Oct 6, 2017

paulirish Oct 9, 2017

patrickhulce Oct 6, 2017

paulirish Oct 9, 2017

patrickhulce Oct 30, 2017

paulirish Oct 31, 2017

brendankenny left a comment

brendankenny commented Oct 6, 2017

brendankenny Oct 6, 2017

patrickhulce Oct 6, 2017

paulirish Oct 9, 2017

paulirish commented Oct 9, 2017

brendankenny commented Oct 10, 2017

brendankenny left a comment

paulirish commented Oct 20, 2017

brendankenny left a comment

brendankenny commented Oct 30, 2017

patrickhulce Oct 30, 2017

patrickhulce left a comment

patrickhulce Oct 30, 2017

report(redirects): reformat results, incl all requests and wasted time, #3492

report(redirects): reformat results, incl all requests and wasted time, #3492

Conversation

paulirish commented Oct 6, 2017 • edited Loading

Changes:

screenshots

Choose a reason for hiding this comment

wardpeet Oct 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wardpeet 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

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

brendankenny commented Oct 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish commented Oct 9, 2017

brendankenny commented Oct 10, 2017

brendankenny left a comment

Choose a reason for hiding this comment

paulirish commented Oct 20, 2017

brendankenny left a comment

Choose a reason for hiding this comment

brendankenny commented Oct 30, 2017

Choose a reason for hiding this comment

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish commented Oct 6, 2017 •

edited

Loading

wardpeet Oct 6, 2017 •

edited

Loading