core(fr): extract storage and service worker driver methods #12400
Conversation
| @@ -51,6 +52,9 @@ class ExecutionContext { | |||
| async _getOrCreateIsolatedContextId() { | |||
| if (typeof this._executionContextId === 'number') return this._executionContextId; | |||
|
|
|||
| await this._session.sendCommand('Page.enable'); | |||
| await this._session.sendCommand('Runtime.enable'); | |||
There was a problem hiding this comment.
before this dependency was an implicit global relationship, felt better to make it explicit.
| }); | ||
| } | ||
|
|
||
| module.exports = {getServiceWorkerVersions, getServiceWorkerRegistrations}; |
There was a problem hiding this comment.
nothing should be changed here
| cleanBrowserCaches, | ||
| getImportantStorageWarning, | ||
| UIStrings, | ||
| }; |
There was a problem hiding this comment.
nothing should be different here either
| * @param {string} pageUrl | ||
| * @return {Promise<void>} | ||
| */ | ||
| static assertNoSameOriginServiceWorkerClients(session, pageUrl) { |
There was a problem hiding this comment.
I moved this here instead of service-workers because it's going to be specific to legacy flow and not shared. We can't assert this in FR where we'll be on the page without any about:blank in between.
Given our neutering of the PWA checks I'm not sure it really has much value for us today anyway.
| const session = driver.defaultSession; | ||
|
|
||
| // Assert no service workers are still installed, so we test that they would actually be installed for a new user. | ||
| // TODO(FR-COMPAT): re-evaluate the necessity of this check |
There was a problem hiding this comment.
see above comment
|
|
||
| // Enable `Debugger` domain to receive async stacktrace information on network request initiators. | ||
| // This is critical for tracing certain performance simulation situations. | ||
| session.on('Debugger.paused', () => session.sendCommand('Debugger.resume')); |
There was a problem hiding this comment.
this will be extracted along with the other shared driver setup in a future PR that focuses on gather-runner
| jest.useRealTimers(); | ||
| }); | ||
|
|
||
| describe('.getServiceWorkerVersions', () => { |
There was a problem hiding this comment.
these weren't tested before 🎉
| @@ -495,41 +596,6 @@ describe('GatherRunner', function() { | |||
| expect(artifacts.devtoolsLogs).toHaveProperty('pageLoadError-nextPass'); | |||
| }); | |||
|
|
|||
| it('does not clear origin storage with flag --disable-storage-reset', () => { | |||
There was a problem hiding this comment.
this test was moved next to the "clears origin storage" test
Which methods did you have in mind? I agree the promise logic in service workers is hard to follow, but they're mostly event based which demand that structure (could remove a Storage methods I believe are all async'd up already. |
|
oh, the only offender was |
| @@ -202,30 +202,6 @@ describe('.beginTrace', () => { | |||
| }); | |||
| }); | |||
|
|
|||
| describe('.setExtraHTTPHeaders', () => { | |||
There was a problem hiding this comment.
Are these tests moved to gather-runner-test.js?
There was a problem hiding this comment.
no the methods have been eliminated, not moved, and so have the tests.
gather runner already had tests that the protocol methods were called correctly so we're good on that front 👍
Co-authored-by: Adam Raine <6752989+adamraine@users.noreply.github.com>
Summary
Splits out the storage and service worker bits from #12366 along with a couple of the super low value driver abstractions. Again the goal here is that FR-shared logic is refactored to accept a session instead of a driver. Ultimately everything left in
driver.jsandgather-runner.jsshould be specific to legacy flow with everything shared living elsewhere.Related Issues/PRs
ref #11313 #12366