-
Notifications
You must be signed in to change notification settings - Fork 823
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
Throwing error instead of re registering sync on failure #1155
Changes from 4 commits
6c79afd
bb02900
2e6c392
6c48088
274bb31
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,6 +23,7 @@ import {DB_NAME, OBJECT_STORE_NAME} from | |
import {DBWrapper} from '../../../../packages/workbox-core/_private/DBWrapper.mjs'; | ||
import {resetEventListeners} from | ||
'../../../../infra/testing/sw-env-mocks/event-listeners.js'; | ||
import {WorkboxError} from '../../../../packages/workbox-core/_private/WorkboxError.mjs'; | ||
|
||
const getObjectStoreEntries = async () => { | ||
return await new DBWrapper(DB_NAME, 1).getAll(OBJECT_STORE_NAME); | ||
|
@@ -293,37 +294,42 @@ describe(`[workbox-background-sync] Queue`, function() { | |
await queue.addRequest(new Request('/three')); | ||
await queue.addRequest(new Request('/four')); | ||
await queue.addRequest(new Request('/five')); | ||
await queue.replayRequests(); // The 2nd and 4th requests should fail. | ||
|
||
|
||
const entries = await getObjectStoreEntries(); | ||
expect(entries.length).to.equal(2); | ||
expect(entries[0].storableRequest.url).to.equal('/two'); | ||
expect(entries[1].storableRequest.url).to.equal('/four'); | ||
try { | ||
await queue.replayRequests(); // The 2nd and 4th requests should fail. | ||
} catch (error) { | ||
const entries = await getObjectStoreEntries(); | ||
expect(entries.length).to.equal(2); | ||
expect(entries[0].storableRequest.url).to.equal('/two'); | ||
expect(entries[1].storableRequest.url).to.equal('/four'); | ||
return; | ||
} | ||
|
||
throw new Error('should have exit from catch'); | ||
}); | ||
|
||
it(`should re-register for a sync event if re-fetching fails`, | ||
it(`should throw WorkboxError if re-fetching fails`, | ||
async function() { | ||
sandbox.stub(self.registration, 'sync').value({ | ||
register: sinon.stub().resolves(), | ||
}); | ||
sandbox.stub(self, 'fetch') | ||
.onCall(1).rejects(new Error()) | ||
.callThrough(); | ||
|
||
const failureURL = '/two'; | ||
const queue = new Queue('foo'); | ||
|
||
// Add requests for both queues to ensure only the requests from | ||
// the matching queue are replayed. | ||
await queue.addRequest(new Request('/one')); | ||
await queue.addRequest(new Request('/two')); | ||
await queue.addRequest(new Request(failureURL)); | ||
|
||
self.registration.sync.register.reset(); | ||
await queue.replayRequests(); // The second request should fail. | ||
try { | ||
await queue.replayRequests(); // The second request should fail. | ||
} catch (error) { | ||
expect(error).to.be.instanceof(WorkboxError); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please use expectError helper here as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
return; | ||
} | ||
|
||
expect(self.registration.sync.register.calledOnce).to.be.true; | ||
expect(self.registration.sync.register.calledWith( | ||
'workbox-background-sync:foo')).to.be.true; | ||
throw new Error('should have exit from catch'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This wont be needed after switching to expectError(). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
}); | ||
|
||
it(`should invoke all replay callbacks`, async function() { | ||
|
@@ -376,7 +382,13 @@ describe(`[workbox-background-sync] Queue`, function() { | |
|
||
await queue.addRequest(new Request('/three')); | ||
await queue.addRequest(new Request('/four')); | ||
await queue.replayRequests(); | ||
try { | ||
await queue.replayRequests(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please use expectError here and make sure the error is what you expect. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
} catch (error) { | ||
// Dont do anything as this is expected to be rejected | ||
// but every callback should still be called. | ||
} | ||
|
||
|
||
expect(requestWillReplay.calledTwice).to.be.true; | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you use the helper: https://github.com/GoogleChrome/workbox/blob/v3/infra/testing/expectError.js
This will check the error is a WorkboxError with an expected code and it returns a promise, so you can await for the response and check the object store entries after.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done