Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tests(smokehouse): use mime-types + proper encoding for response write #9542

Merged
merged 3 commits into from
Aug 9, 2019
Merged

tests(smokehouse): use mime-types + proper encoding for response write #9542

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c8a079b
tests(smokehouse): use mime-types for content-type. use proper encodi…
connorjclark Aug 9, 2019
45b2a92
bytes
connorjclark Aug 9, 2019
7209515
Update lighthouse-cli/test/fixtures/static-server.js
connorjclark Aug 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 12 additions & 20 deletions lighthouse-cli/test/fixtures/static-server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ const http = require('http');
const zlib = require('zlib');
const path = require('path');
const fs = require('fs');
const mime = require('mime-types');
const parseQueryString = require('querystring').parse;
const parseURL = require('url').parse;
const URLSearchParams = require('url').URLSearchParams;
Expand Down Expand Up @@ -63,23 +64,13 @@ function requestHandler(request, response) {
function sendResponse(statusCode, data) {
const headers = {'Access-Control-Allow-Origin': '*'};

if (filePath.endsWith('.js')) {
headers['Content-Type'] = 'text/javascript';
} else if (filePath.endsWith('.css')) {
headers['Content-Type'] = 'text/css';
} else if (filePath.endsWith('.svg')) {
headers['Content-Type'] = 'image/svg+xml';
} else if (filePath.endsWith('.png')) {
headers['Content-Type'] = 'image/png';
} else if (filePath.endsWith('.gif')) {
headers['Content-Type'] = 'image/gif';
} else if (filePath.endsWith('.jpg') || filePath.endsWith('.jpeg')) {
headers['Content-Type'] = 'image/jpeg';
} else if (filePath.endsWith('.webp')) {
headers['Content-Type'] = 'image/webp';
} else if (filePath.endsWith('.json')) {
headers['Content-Type'] = 'application/json';
}
const contentType = mime.lookup(filePath);
const charset = mime.lookup(contentType);
// `mime.contentType` appends the correct charset too.
// Note: it seems to miss just one case, svg. Doesn't matter much, we'll just allow
// svgs to fallback to binary encoding. `Content-Type: image/svg+xml` is sufficient for our use case.
// see https://github.com/jshttp/mime-types/issues/66
if (contentType) headers['Content-Type'] = mime.contentType(contentType);

let delay = 0;
let useGzip = false;
Expand Down Expand Up @@ -127,7 +118,8 @@ function requestHandler(request, response) {
return setTimeout(finishResponse, delay, data);
}

finishResponse(data);
const encoding = charset === 'UTF-8' ? 'utf-8' : 'binary';
finishResponse(data, encoding);
}

function sendRedirect(url) {
Expand All @@ -138,8 +130,8 @@ function requestHandler(request, response) {
response.end();
}

function finishResponse(data) {
response.write(data, 'binary');
function finishResponse(data, encoding) {
response.write(data, encoding);
response.end();
}
}
Expand Down
4 changes: 2 additions & 2 deletions lighthouse-cli/test/smokehouse/byte-efficiency/expectations.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,12 +132,12 @@ module.exports = [
},
{
url: 'http://localhost:10200/byte-efficiency/script.js?gzip=1',
transferSize: 1136,
transferSize: 1158,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just from content-encoding header?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

> 'content-encoding:utf-8'.length
22
> 1158 -1136
22

math checks out 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yay, someone did the maths

resourceSize: 52997,
},
{
url: 'http://localhost:10200/byte-efficiency/script.js',
transferSize: 53181,
transferSize: 53203,
resourceSize: 52997,
},
{
Expand Down
4 changes: 2 additions & 2 deletions lighthouse-cli/test/smokehouse/perf/expectations.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,8 @@ module.exports = [
{resourceType: 'font', requestCount: 2, size: '80000±1000'},
{resourceType: 'script', requestCount: 3, size: '55000±1000'},
{resourceType: 'image', requestCount: 2, size: '28000±1000'},
{resourceType: 'document', requestCount: 1, size: '2100±100'},
{resourceType: 'other', requestCount: 2, size: '1250±50'},
{resourceType: 'document', requestCount: 1, size: '2200±100'},
Copy link
Collaborator Author

@connorjclark connorjclark Aug 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These numbers went up because Content-Type either was added where it previously wasn't present, or it got larger.

All these files were either sending ascii (the documents), which doesn't encode larger in utf-8 than plain binary - or were sending binary (whose content size also wouldn't have changed). So it had to be the headers.

{resourceType: 'other', requestCount: 2, size: '1300±50'},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

really wish we knew what these two other requests were right about now...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fonts 🦀 🦀 🦀

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but we breakout fonts into their own thing already?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going off memory here. I'll fire it up...

image

ok yeah i was mistaken. fetch and the favicon

(sidenote - I want to add a favicon to the static server eventually, it keeps coming up as a "difference" when running smoke tests in different environments. Only in the CLI case are we attached to the protocol while a favicon is requested - in LR and DT we connect too late).

Copy link
Collaborator Author

@connorjclark connorjclark Aug 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if a "fetch" should dig a little deeper for categorizing? Fetching that .html document could possibly be considered a document resource.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah that's certainly interesting, I would consider fetch worthy of an XHR/API category all its own anyhow but probably a topic worth bringing up in a different thread :)

want to throw the comment on there that it's the favicon and fetch for now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

{resourceType: 'stylesheet', requestCount: 1, size: '450±100'},
{resourceType: 'media', requestCount: 0, size: 0},
{resourceType: 'third-party', requestCount: 0, size: 0},
Expand Down