Skip to content

Add tests of MethodModelingViewProvider.viewLoaded#3444

Merged
robertbrignull merged 3 commits intomainfrom
robertbrignull/start-modeling-test
Mar 11, 2024
Merged

Add tests of MethodModelingViewProvider.viewLoaded#3444
robertbrignull merged 3 commits intomainfrom
robertbrignull/start-modeling-test

Conversation

@robertbrignull
Copy link
Copy Markdown
Contributor

This PR is trying to add tests that would have caught the bug fixed in #3439

To do this we need to initialize a MethodModelingViewProvider and send it a "viewLoaded" message. Then we can test that it does the right thing in various ModelingStore states. I believe the only thing the test can verify is what messages are sent to the webview.

I've focussed on what it does when the view is first loaded and haven't tried to add tests for all the other behaviours of MethodModelingViewProvider. We could add more tests later if we think that would be useful.

Checklist

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

@robertbrignull robertbrignull requested a review from a team as a code owner March 7, 2024 10:27
Copy link
Copy Markdown
Member

@koesie10 koesie10 left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +29 to +33
let getStateForActiveDb: jest.Mock<DbModelingState | undefined, []>;
let getSelectedMethodDetails: jest.Mock<
SelectedMethodDetails | undefined,
[]
>;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would it make sense to use the function definitions here rather than manually specifying the arguments and return types?

Suggested change
let getStateForActiveDb: jest.Mock<DbModelingState | undefined, []>;
let getSelectedMethodDetails: jest.Mock<
SelectedMethodDetails | undefined,
[]
>;
let getStateForActiveDb: jest.MockedFunction<
ModelingStore["getStateForActiveDb"]
>;
let getSelectedMethodDetails: jest.MockedFunction<
ModelingStore["getSelectedMethodDetails"]
>;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Nice. I wasn't aware of jest.MockedFunction but that is a lot cleaner.

Comment on lines +93 to +94
mockedObject<WebviewViewResolveContext>({}),
new CancellationTokenSource().token,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It seems like it should be safe to remove these parameters from the resolveWebviewView method implementation so you don't need to pass these.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

resolveWebviewView is implementing a method from the WebviewViewProvider provider so that's why it had those unused arguments. They're there for documentation purposes, though personally I agree I think it's fine to not define them on our definition if we're not using them.

Previously it was just a matter of documentation preference with no other concerns, but now that we're calling this method ourselves in the tests it's making our testing harder as we have to mock more objects. So I think that's enough justification to remove them now.

@robertbrignull robertbrignull requested a review from a team as a code owner March 11, 2024 11:05
@robertbrignull robertbrignull merged commit 650b09c into main Mar 11, 2024
@robertbrignull robertbrignull deleted the robertbrignull/start-modeling-test branch March 11, 2024 11:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants