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: fix flaky cache test #12606

Merged
merged 2 commits into from
Jun 2, 2021
Merged

tests: fix flaky cache test #12606

merged 2 commits into from
Jun 2, 2021

Conversation

connorjclark
Copy link
Collaborator

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
@@ -116,6 +116,11 @@ describe('Cache headers audit', () => {
});

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

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

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 :)

@@ -116,6 +116,11 @@ describe('Cache headers audit', () => {
});

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

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

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

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

@brendankenny
Copy link
Member

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

@brendankenny
Copy link
Member

brendankenny commented Jun 2, 2021

it seems like from

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

// 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

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

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.

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