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.

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: add more cases for oopif smoke test #13705

Merged
merged 17 commits into from
Mar 1, 2022
Merged

Conversation

connorjclark
Copy link
Collaborator

I didn't understand under what circumstances we determine a script (via ScriptElement) to be from an OOPIF (when the network record has a sessionId thus is not from the main target).

Learned some things:

  • iframes from the same domain (even different ports) will be in the same target, thus don't get filtered
  • but workers are a different target (with a sessionId) so it's considered an "OOPIF" and all its scripts are blocked
  • ditto for service workers (but I didn't bother adding a test case for that)

Adding more test cases to capture how things change if we decide to stop filtering "OOPIF" scripts from our audits.

@connorjclark connorjclark requested a review from a team as a code owner February 25, 2022 02:00
@connorjclark connorjclark requested review from adamraine and removed request for a team February 25, 2022 02:00
@connorjclark connorjclark changed the title test: add more cases for oopif smoke test tests: add more cases for oopif smoke test Feb 25, 2022
{url: 'http://localhost:10503/oopif-simple-page.html'},
// simple-script.js is included many times
// 2 * (1 from <script>, 1 from fetch) = 4
// Note, the network records from the workers are _not_ captured!
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

bug alert! ⚠️

lighthouse-cli/test/smokehouse/report-assert.js Outdated Show resolved Hide resolved
const unexpectedValues = expectedValue;
console.log({arrToCheckAgainst});
for (const unexpectedEntry of unexpectedValues) {
const matchingIndex = arrToCheckAgainst.findIndex(actualEntry =>
Copy link
Member

Choose a reason for hiding this comment

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

We don't do any splicing, can probably just use arrToCheckAgainst.find

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hm... but then there's the case of undefined being a valid value in these arrays :P

Copy link
Member

Choose a reason for hiding this comment

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

Ah ya, index works fine then

lighthouse-cli/test/smokehouse/readme.md Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants