Skip to content

tests: fix flaky cache test #12606

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
Merged
connorjclark merged 2 commits into from
Jun 2, 2021
Merged

tests: fix flaky cache test #12606

connorjclark merged 2 commits into from
Jun 2, 2021

Conversation

@connorjclark
Copy link
Collaborator

@connorjclark connorjclark commented Jun 2, 2021

fixes #11772

@connorjclark connorjclark requested a review from a team as a code owner June 2, 2021 19:57
@connorjclark connorjclark requested review from patrickhulce and removed request for a team June 2, 2021 19:57
@google-cla google-cla bot added the cla: yes label Jun 2, 2021
connorjclark
connorjclark commented Jun 2, 2021
View reviewed changes
lighthouse-core/test/audits/byte-efficiency/uses-long-cache-ttl-test.js Outdated

it('respects expires/cache-control priority', () => {
// Stub Date.now so the test is not sensitive to timing.
const now = Date.now();
Copy link
Collaborator Author

@connorjclark connorjclark Jun 2, 2021

Choose a reason for hiding this comment

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

could make the argument that this should be in a beforeAll. thoughts?

Copy link
Collaborator

@patrickhulce patrickhulce Jun 2, 2021

Choose a reason for hiding this comment

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

I would make that argument yes :)

patrickhulce
patrickhulce suggested changes Jun 2, 2021
View reviewed changes
lighthouse-core/test/audits/byte-efficiency/uses-long-cache-ttl-test.js Outdated

it('respects expires/cache-control priority', () => {
// Stub Date.now so the test is not sensitive to timing.
const now = Date.now();
Copy link
Collaborator

@patrickhulce patrickhulce Jun 2, 2021

Choose a reason for hiding this comment

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

I would make that argument yes :)

@patrickhulce
Copy link
Collaborator

patrickhulce commented Jun 2, 2021 •
edited
Loading

wait hang on, how does this fix the flake...? isn't the problem that too much time elapses between now and the assertion which is still true? nevermind I gotcha :)

@connorjclark
Copy link
Collaborator Author

connorjclark commented Jun 2, 2021

how does time pass if date.now is constant. sppoky time

@brendankenny
Copy link
Contributor

brendankenny commented Jun 2, 2021

so no to switching the audit off of Date and to request timing?

@brendankenny
Copy link
Contributor

brendankenny commented Jun 2, 2021 •
edited
Loading

it seems like from

lighthouse/lighthouse-core/audits/byte-efficiency/uses-long-cache-ttl.js

Lines 118 to 124 in a938fc4

const expiresHeaders = headers.get('expires');
if (expiresHeaders) {
const expires = new Date(expiresHeaders).getTime();
// Invalid expires values MUST be treated as already expired
if (!expires) return 0;
return Math.ceil((expires - Date.now()) / 1000);
}

auditing with -A would often mean headers hitting this branch would have a negative cacheLifetimeInSeconds, which we then discard down in

lighthouse/lighthouse-core/audits/byte-efficiency/uses-long-cache-ttl.js

Lines 222 to 228 in a938fc4

// Ignore if cacheLifetimeInSeconds is a nonpositive number.
let cacheLifetimeInSeconds = CacheHeaders.computeCacheLifetimeInSeconds(
headers, cacheControl);
if (cacheLifetimeInSeconds !== null &&
(!Number.isFinite(cacheLifetimeInSeconds) || cacheLifetimeInSeconds <= 0)) {
continue;
}

so would never show up in the audit?

I realize this is expanding the scope of the intended fix, so this can also be a new issue :)

@connorjclark
Copy link
Collaborator Author

connorjclark commented Jun 2, 2021

merging in interest of less flaky tests. i'll write a new issue but can't take it right away.

@connorjclark connorjclark merged commit 7a4a317 into master Jun 2, 2021
@connorjclark connorjclark deleted the fixflakyyaytatertots branch June 2, 2021 20:53
@connorjclark connorjclark mentioned this pull request Jun 2, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@patrickhulce patrickhulce patrickhulce approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@patrickhulce patrickhulce

Labels

cla: yes waiting4committer

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

flaky test in uses-long-cache-ttl-test.js

4 participants

@connorjclark @patrickhulce @brendankenny @devtools-bot