Skip to content
This repository has been archived by the owner on Aug 31, 2021. It is now read-only.

Refactors the tests of cache expiration options. #187

Merged
merged 3 commits into from
Sep 22, 2016

Conversation

jeffposnick
Copy link
Contributor

R: @gauntface
CC: @addyosmani

This fixes up a test that was flaky due to the need to clear out IDB in between invocations, which didn't always succeed due to the DB instance being held open in the service worker.

It's a refactoring of the logic so that it's no longer necessary to clear out IDB, since there's a separate DB instance used for each individual sub-test (scoped to the service worker registration).

I also took the opportunity to refactor some of the promise chains to make them more readable, and move common logic out to helper functions.

Copy link

@gauntface gauntface left a comment

Choose a reason for hiding this comment

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

Apart from the one open Q. LGTM, looks like a good change.

const assertCacheContents = (cachedAssets, expectedUrls) => {
const expectedLength = expectedUrls.length;
const cachedUrls = Object.keys(cachedAssets);
const filteredUrls = cachedUrls.filter(url => expectedUrls.includes(url));
Copy link

@gauntface gauntface Sep 22, 2016

Choose a reason for hiding this comment

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

Not sure of when filteredUrls != cachedUrls and that would be OK.

Would it make sense to invert the includes check and ensure filteredUrls.length === 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of the tests in this file assume that the cached contents need to be exactly equal to the expected contents. I.e., if there's something in cached contents that's not in expected, or vice versa, then it's a failure. This logic seemed like the easiest way to check that bijection.

It's not really a general-purpose check that's applicable to other tests, since other tests might be implemented in such a way that extra entries in the cached contents that aren't expected wouldn't be considered a failure.

@jeffposnick jeffposnick merged commit 41133c2 into master Sep 22, 2016
@jeffposnick jeffposnick deleted the clean-up-idb-in-test branch September 22, 2016 16:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants