Skip to content

Add some basic integration tests for MRVA#1665

Merged
koesie10 merged 3 commits intomainfrom
koesie10/msw-integration-test
Oct 31, 2022
Merged

Add some basic integration tests for MRVA#1665
koesie10 merged 3 commits intomainfrom
koesie10/msw-integration-test

Conversation

@koesie10
Copy link
Copy Markdown
Member

This adds some basic integration tests for MRVA using the GitHub mock API server. It only does basic assertions and still needs to stub some things because it is quite hard to properly test things since VSCode does not expose an API to e.g. answer quick pick pop-ups.

I'm not sure how useful these integration tests will actually be in practice, but they do at least ensure that we are able to successfully submit a variant analysis.

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.

This adds some basic integration tests for MRVA using the GitHub mock
API server. It only does basic assertions and still needs to stub some
things because it is quite hard to properly test things since VSCode
does not expose an API to e.g. answer quick pick pop-ups.

I'm not sure how useful these integration tests will actually be in
practice, but they do at least ensure that we are able to successfully
submit a variant analysis.
@koesie10 koesie10 requested a review from charisk October 27, 2022 11:09
@koesie10 koesie10 marked this pull request as ready for review October 27, 2022 11:09
@koesie10 koesie10 requested a review from a team as a code owner October 27, 2022 11:09
Copy link
Copy Markdown
Contributor

@charisk charisk left a comment

Choose a reason for hiding this comment

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

Thanks for exploring this!

I think it's worth getting these tests in as variant analysis submission tests that happen to use the mock API rather than suggesting they're special in some way (aka "variant analysis integration tests"). What do you think? Appreciate I'm probably the one that suggested the current term in the first place 😬

return document;
}

describe('Variant Analysis Integration', function() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I guess these are tests around variant analysis submission. What do you think?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, that makes sense. The reason for calling it an integration test is that they are not linked to a specific class, so if I had just named the file variant-analysis.test.ts I'd expect it to test a file variant-analysis.ts. To differentiate, I added .integration.test.ts to ensure that the difference is clear. What do you think?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think once we start talking about integration of things it makes sense to not have a 1-to-1 mapping between tests and classes. We're testing the integration between multiple things to prove out a feature or part of it. In this case we're testing variant analysis submission.

So perhaps variant-analysis-submission-integration.test.ts?

Copy link
Copy Markdown
Contributor

@charisk charisk left a comment

Choose a reason for hiding this comment

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

LGTM!

@koesie10 koesie10 merged commit 25a9ee1 into main Oct 31, 2022
@koesie10 koesie10 deleted the koesie10/msw-integration-test branch October 31, 2022 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants