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

Jump to bottom

tests: use origin-agent-cluster to actually test oopifs #13777

Merged
merged 5 commits into from
Apr 1, 2022
Merged

tests: use origin-agent-cluster to actually test oopifs #13777

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0746507
tests: use origin-agent-cluster to actually test oopifs
connorjclark Mar 23, 2022
2dc9b7f
Merge branch 'master' into oopif-agent-cluster
connorjclark Mar 30, 2022
c6a2dd6
fix
connorjclark Mar 31, 2022
5ea909c
Merge branch 'oopif-agent-cluster' of github.com:GoogleChrome/lightho…
connorjclark Mar 31, 2022
5b8a4f5
pr
connorjclark Mar 31, 2022
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
2 changes: 2 additions & 0 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 @@ -110,6 +110,8 @@ class Server {
// see https://github.com/jshttp/mime-types/issues/66
if (contentType) headers['Content-Type'] = mime.contentType(contentType);

headers['origin-agent-cluster'] = ['?1'];
Copy link
Member

Choose a reason for hiding this comment

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

Should we put this behind our own url parameter like isolated_frame or something? This is non-default behavior that we only use for one test.

If not could you add this to L103.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

couple reasons why I didn't put this behind a query param:

  • chrome doesn't like it when you mix responses from the same origin with and without this header. I found I had to reset chrome to undo this "mix responses" state when navigating to these pages directly via yarn static-server, which was pretty confusing
  • At least for me, I always assumed iframing across different ports on localhost would be isolated. Probably best if just isolate by default for our smoke tests.


let delay = 0;
let useGzip = false;
if (queryString) {
Expand Down
15 changes: 2 additions & 13 deletions lighthouse-cli/test/smokehouse/test-definitions/oopif-scripts.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,19 +102,16 @@ const expectations = {
isPositionFixed: true,
},
],
// Only `oopif-simple-page.html`'s inclusion of `simple-script.js` should show up here.
// Only `:10200/oopif-simple-page.html`'s inclusion of `simple-script.js` shows here.
// All other scripts are filtered out because of our "OOPIF" filter (including anything
// that is just in another process, like a worker).
ScriptElements: [
{
src: 'http://localhost:10200/simple-script.js',
source: 'network',
},
{
Copy link
Collaborator Author

@connorjclark connorjclark Mar 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

shouldn't have ignored this in the original pr :)

src: 'http://localhost:10503/simple-script.js',
source: 'network',
},
],
// Same here, except we get inline scripts of the iframe.
Scripts: {
_includes: [
{
Expand All @@ -125,14 +122,6 @@ const expectations = {
url: 'http://localhost:10200/oopif-simple-page.html',
content: /new Worker/,
},
{
url: 'http://localhost:10503/simple-script.js',
content: /🪁/,
},
{
url: 'http://localhost:10503/oopif-simple-page.html',
content: /new Worker/,
},
],
_excludes: [{}],
},
Expand Down