-
Notifications
You must be signed in to change notification settings - Fork 29
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
Add command to reset state #3421
Conversation
Not really sure how to test this to be honest. |
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.
Not really sure how to test this to be honest.
Could have an external function that is registered in the extension. Just give the function the workspace state instead of the whole context. You could then test that function with a mockMemento
to make sure all of the correct keys are called. the main reason for adding that test would be to document the behaviour.
Maybe add extension/src/persistence/util.ts
& extension/src/persistence/register.ts
. Register would take the workspaceState
object and so would the util. The test could be done on the util (wouldn't test the registered function as it would cause unexpected result in development)... remember to stub reload window 😁
edit: this is a suggestion.
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.
Looks good!
Thanks that was really useful. |
Code Climate has analyzed commit 93dde1f and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 92.8% (85% is the threshold). This pull request will bring the total coverage in the repository to 95.7% (0.0% change). View more on Code Climate. |
}) | ||
update: (key: string, value: unknown) => { | ||
values[key] = value | ||
void Promise.resolve() |
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.
[Q] Should this be returned?
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.
I don't think we ever assign the result. Also there is no result to ever be returned. From there, I think it's more of a stylistic choice.
Screen.Recording.2023-03-07.at.5.26.22.PM.mov