HTTP/2 test incorrectly passes

d-chen commented 7 years ago

I see a similar report in #163 but I wasn't sure if it'd be okay to ping people on an old issue.

I was demoing the Coach extension in Chrome and someone pointed out the HTTPS test correctly failed but was still passing HTTP/2. Also when running the sitespeed.io NPM package, I'm not seeing a HTTP/2 recommendation under the Coach tab.

Is this test intended to not deduct points if a website is using HTTP? Currently the test name/description is giving us the impression it's a false positive.

  var score = 100;

  if (url.indexOf('https://') > -1 && connectionType.indexOf('h2') === -1) {
    score = 0;
    message = 'The page is using HTTPS but not HTTP/2. Change to HTTP/2 to follow new best practice and make the site faster.';
  }

Scores for each scenario:

var score = (url, connectionType) => {
  let value = (url.indexOf('https://') > -1 && connectionType.indexOf('h2') === -1) ? 0 : 100;
  console.log(value);
}

score('http://', 'http/1.1');  // 100 => ???
score('http://', 'h2');        // 100; won't ever go this path. browsers run HTTP/2 over HTTPS
score('https://', 'http/1.1'); // 0; penalized for omitting HTTP/2
score('https://', 'h2');       // 100; pass for using HTTP/2

soulgalore commented 7 years ago

Hey @d-chen thanks for reporting. I think the idea has been that if you are running https you should always upgrade to HTTP/2. However I think we should be revisited and change the advice since HTTP/2 is not always faster (better that it compress headers but not always faster). The advice should be that you need to measure and choose yourself.

HTTPS is important (there's another advice for that with higher prio), HTTP/2 has low but we should still change the text.

Let me update the advice (and go through the rest we have) the coming days.

Best Peter

soulgalore commented 5 years ago

Oops this took time to fix. This is fixed in coming 3.0 though.

sitespeedio / coach

HTTP/2 test incorrectly passes #269