-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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.
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
Conversation
…ng for response write
@@ -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'}, |
There was a problem hiding this comment.
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.
@@ -132,12 +132,12 @@ module.exports = [ | |||
}, | |||
{ | |||
url: 'http://localhost:10200/byte-efficiency/script.js?gzip=1', | |||
transferSize: 1136, | |||
transferSize: 1158, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 👍
There was a problem hiding this comment.
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
{resourceType: 'document', requestCount: 1, size: '2100±100'}, | ||
{resourceType: 'other', requestCount: 2, size: '1250±50'}, | ||
{resourceType: 'document', requestCount: 1, size: '2200±100'}, | ||
{resourceType: 'other', requestCount: 2, size: '1300±50'}, |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fonts 🦀 🦀 🦀
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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...
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Co-Authored-By: Patrick Hulce <patrick.hulce@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This unlocks the power of emojis in our smoke tests :)
Must properly set the
encoding
param if sending utf-8 data. Default wasbinary
, which drops non-ascii characters.Tossed in usage of
mime-types
(already in our sub-dependencies) because I found myself adding yet another condition (.woff) ...