-
Notifications
You must be signed in to change notification settings - Fork 28
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
Run remote experiments data update in the background #4342
Conversation
cf6cccb
to
c8a816a
Compare
@@ -58,15 +66,13 @@ export abstract class BaseData< | |||
this.staticFiles = staticFiles | |||
|
|||
this.watchFiles() | |||
|
|||
this.waitForInitialData() |
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.
[F] Had to rework this because we are now sending multiple notifications per update for experiments.
@@ -35,23 +39,18 @@ export class ExperimentsData extends BaseData<ExperimentsOutput> { | |||
|
|||
void this.watchExpGitRefs() | |||
void this.managedUpdate() | |||
this.waitForInitialLocalData() |
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.
[F] Only care about the local data when initializing this class
if (isRemoteExperimentsOutput(data)) { | ||
const { remoteExpRefs } = data | ||
this.experiments.transformAndSetRemote(remoteExpRefs) | ||
return this.webviewMessages.sendWebviewMessage() |
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.
[F] Currently the only place that the remote data is needed
expect(managedUpdateSpy).to.be.called | ||
|
||
await Watcher.fireWatcher(mockDotGitNestedFilePath) | ||
await Watcher.fireWatcher(mockDotGitFilePath) | ||
await dataUpdatedEvent | ||
|
||
expect(managedUpdateSpy).to.be.called |
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.
[F] Because this is the same spy and the same event this part of the test didn't test anything. Can revisit later.
@@ -654,6 +654,7 @@ suite('Workspace Experiments Test Suite', () => { | |||
bypassProgressCloseDelay() | |||
const { experiments } = stubWorkspaceExperimentsGetters(disposable) | |||
await experiments.isReady() | |||
stub(experiments, 'update').resolves(undefined) |
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.
[F] This caused some log spam
c8a816a
to
f20fdd3
Compare
f20fdd3
to
1b92f03
Compare
Code Climate has analyzed commit 1b92f03 and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 93.0% (85% is the threshold). This pull request will bring the total coverage in the repository to 95.2% (0.0% change). View more on Code Climate. |
Follow up to #4324 (comment)
Demo
Screen.Recording.2023-07-25.at.1.12.44.pm.mov
Screen.Recording.2023-07-25.at.1.16.43.pm.mov