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

deps(angular): update minor version of angular fixture #11043

Closed
wants to merge 3 commits into from
Closed

deps(angular): update minor version of angular fixture #11043

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
e8196ef
deps(angular): update minor version of angular fixture
paulirish Jun 30, 2020
304c352
Update lighthouse-core/test/lib/minification-estimator-test.js
paulirish Jun 30, 2020
7affb58
rename
paulirish Jun 30, 2020
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
13 changes: 10 additions & 3 deletions lighthouse-core/test/lib/minification-estimator-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ const assert = require('assert').strict;
const {computeCSSTokenLength, computeJSTokenLength} = require('../../lib/minification-estimator.js'); // eslint-disable-line max-len

const angularFullScript = fs.readFileSync(require.resolve('angular/angular.js'), 'utf8');
const zoneMinifiedScript = fs.readFileSync(`${__dirname}/../../../lighthouse-cli/test/fixtures/dobetterweb/third_party/aggressive-promise-polyfill.js`, 'utf8'); // eslint-disable-line max-len
Copy link
Member

Choose a reason for hiding this comment

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

can we use this as motivation to move aggressive-promise-polyfill.js to the root third-party/ where it probably should be? :)

Copy link
Member Author

Choose a reason for hiding this comment

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

i looked into it, but

lighthouse/lighthouse-cli/test/fixtures/static-server.js

Line 67 in 1330133

let absoluteFilePath = path.join(__dirname, filePath);

makes that pretty challenging. ideas?

Copy link
Member Author

@paulirish paulirish Jul 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

we could do another of these

lighthouse/lighthouse-cli/test/fixtures/static-server.js

Lines 161 to 164 in 1330133

if (filePath.startsWith('/dist/viewer')) {
// Rewrite lighthouse-viewer paths to point to that location.
absoluteFilePath = path.join(__dirname, '/../../../', filePath);
}

🤮

Copy link
Collaborator

Choose a reason for hiding this comment

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

plz no this breaks smoke rider

Copy link
Member

Choose a reason for hiding this comment

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

plz no this breaks smoke rider

Can we talk solutions, then? How did smokerider work when we used to serve it out of node_modules/?

Copy link
Member

Choose a reason for hiding this comment

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

bump

Copy link
Collaborator

@connorjclark connorjclark Jul 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Can we talk solutions, then? How did smokerider work when we used to serve it out of node_modules/?

we skip urls that use external resources, so I guess we skipped that one too when it served a file from node_modules. search for usesExternalResources in google3 smokerider.

can we check it into fixtures/third_party instead, so there is no serving rewiring needed?

EDIT: ah, I see that is how it is now but brendan is suggesting moving to root. so my take is to keep it where it is. but I'd prefer fixtures/third_party as a central place for all possible 3p smoke assets

Copy link
Member

Choose a reason for hiding this comment

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

so my take is to keep it where it is. but I'd prefer fixtures/third_party as a central place for all possible 3p smoke assets

well, those are equally wrong places to put third-party code, so I'm equally fine with them :P

Honestly we should probably delete this file and replace it with a considerably simpler promise stomper (we have a lot more confidence in our isolated context and cached globals than we did when we added this test), but adding it to the unit test is probably going to make it so that never happens :)

Copy link
Collaborator

@connorjclark connorjclark Jul 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

devtools frontend does this (/third_party and /front_end/third_party), so it's not without prior art

Copy link
Member Author

Choose a reason for hiding this comment

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

resolution: we handwrite a promisepolyfill for that test.
we get rid of this big one.
maybe use some other minified js for this estimator test.

@paulirish to do


/* eslint-env jest */

Expand Down Expand Up @@ -214,9 +215,15 @@ describe('minification estimator', () => {
});

it('should handle large, real javscript files', () => {
assert.equal(angularFullScript.length, 1371888);
// 1 - 334968 / 1364217 = estimated 75% smaller minified
assert.equal(computeJSTokenLength(angularFullScript), 337959);
assert.equal(angularFullScript.length, 1374505);
// 1 - 338528 / 1374505 = estimated 75.3% smaller minified
assert.equal(computeJSTokenLength(angularFullScript), 338528);
});

it('should handle real already-minified javscript files', () => {
assert.equal(zoneMinifiedScript.length, 25073);
// 1 - 24119 / 25073 = estimated 3.8% smaller minified
assert.equal(computeJSTokenLength(zoneMinifiedScript), 24119);
});
});
});
6 changes: 3 additions & 3 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -950,9 +950,9 @@ ajv@^5.2.3, ajv@^5.3.0:
json-schema-traverse "^0.3.0"

angular@^1.7.4:
version "1.7.9"
resolved "https://registry.yarnpkg.com/angular/-/angular-1.7.9.tgz#e52616e8701c17724c3c238cfe4f9446fd570bc4"
integrity sha512-5se7ZpcOtu0MBFlzGv5dsM1quQDoDeUTwZrWjGtTNA7O88cD8TEk5IEKCTDa3uECV9XnvKREVUr7du1ACiWGFQ==
version "1.8.0"
resolved "https://registry.yarnpkg.com/angular/-/angular-1.8.0.tgz#b1ec179887869215cab6dfd0df2e42caa65b1b51"
integrity sha512-VdaMx+Qk0Skla7B5gw77a8hzlcOakwF8mjlW13DpIWIDlfqwAbSSLfd8N/qZnzEmQF4jC4iofInd3gE7vL8ZZg==

ansi-align@^2.0.0:
version "2.0.0"
Expand Down