Add tests for variant analysis history item#1650
Merged
elenatanasoiu merged 12 commits intomainfrom Oct 31, 2022
Merged
Conversation
norascheuch
force-pushed
the
nora-elena/integration-tests
branch
from
e64a7fb to
769916e
Compare
elenatanasoiu
force-pushed
the
nora-elena/integration-tests
branch
2 times, most recently
from
c180176 to
8e84ddb
Compare
norascheuch
force-pushed
the
nora-elena/integration-tests
branch
from
0b7ccfd to
e4f3b56
Compare
elenatanasoiu
force-pushed
the
nora-elena/integration-tests
branch
2 times, most recently
from
be9acef to
5427260
Compare
elenatanasoiu
changed the title
elenatanasoiu
force-pushed
the
nora-elena/integration-tests
branch
from
5427260 to
0ed063e
Compare
Contributor
|
Depends on these PRs getting merged in first: |
3 tasks
elenatanasoiu
force-pushed
the
nora-elena/integration-tests
branch
from
0ed063e to
7314c4d
Compare
We will need to set up some VariantAnalysisHistoryItem types in order to use them in our tests. We're repeating what we've done for RemoteQueryHistoryItem for now. Separately we'll think about setting up tests that check for both remote queries and variant analysis in the query history. At the moment we'd like to focus on just adding some test coverage for variant analysis history items. Co-authored-by: Nora Scheuch <norascheuch@github.com>
Co-authored-by: Nora Scheuch <norascheuch@github.com>
Co-authored-by: Nora Scheuch <norascheuch@github.com>
elenatanasoiu
force-pushed
the
nora-elena/integration-tests
branch
from
7314c4d to
9c22ed6
Compare
Contributor
|
(just closed and re-opened in an attempt to kick off PR checks!) |
We've merged #1656 which actually implements item removal. We'll need to change our tests to account for this. We've also merged #1654 which implements opening the view when we click on a variant analysis history item. So we've changed our tests to take into account that there's now a `showView` method being called.
elenatanasoiu
force-pushed
the
nora-elena/integration-tests
branch
from
9c22ed6 to
ad7a04e
Compare
koesie10
approved these changes
Oct 28, 2022
Member
koesie10
left a comment
koesie10
left a comment
There was a problem hiding this comment.
LGTM, I have some small improvement suggestions but this will definitely increase our test coverage
Comment on lines
+130
to
+136
| expect(rehydrateVariantAnalysisStub).to.have.callCount(2); | ||
|
|
||
| expect(rehydrateVariantAnalysisStub.getCall(0).args[0]).to.deep.eq(rawQueryHistory[0].variantAnalysis); | ||
| expect(rehydrateVariantAnalysisStub.getCall(0).args[1]).to.deep.eq(rawQueryHistory[0].status); | ||
|
|
||
| expect(rehydrateVariantAnalysisStub.getCall(1).args[0]).to.deep.eq(rawQueryHistory[1].variantAnalysis); | ||
| expect(rehydrateVariantAnalysisStub.getCall(1).args[1]).to.deep.eq(rawQueryHistory[1].status); |
Member
There was a problem hiding this comment.
These seem like they are testing the rehydration, rather than the removal. Can we remove these and expect that they are tested by the test above?
Contributor
There was a problem hiding this comment.
Yeah they are kinda redundant. I've removed the extra checks.
Comment on lines
+143
to
+144
| expect(removeVariantAnalysisStub).to.have.callCount(1); | ||
| expect(rehydrateVariantAnalysisStub).to.have.callCount(2); |
Member
There was a problem hiding this comment.
Perhaps we can reset these stubs and check that they have call count 0? That would be more resilient to changes.
Contributor
There was a problem hiding this comment.
We call sandbox.restore() in the beforeEach() block, so the count is reset for each of the tests.
I've double checked this by adding these lines at the top of this test:
expect(removeVariantAnalysisStub).to.have.callCount(0);
expect(rehydrateVariantAnalysisStub).to.have.callCount(0);
They passed, which indicates the count starts from zero.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🚨 Please review this PR commit-by-commit.
We're repeating what we've done for the
RemoteQueryHistoryItemtests for now.Separately we'll think about setting up tests that check for both remote queries and variant analysis in the query history.
At the moment we'd like to focus on just adding some test coverage for
VariantAnalysisHistoryItem.Checklist
ready-for-doc-reviewlabel there.