Make query history tests work with remote queries & variant analyses#1688
Merged
elenatanasoiu merged 17 commits intomainfrom Nov 2, 2022
Merged
Conversation
98a08fb to
ab8f59b
Compare
At the moment our query history tests are set up to only check local queries. Let's prepare the ground to introduce remote query history items and variant analysis history items. This will allow us to expand test coverage for these other types of items.
There's a lot of clean-up in these tests so I'm making one change per commit. Let's move out the utility methods so we can focus on just our tests.
There are a couple of tests that check whether we can correctly compare two local queries. These shouldn't be applied to remote queries [1] so let's just make that a bit clearer by moving them into a local queries describe block and using the `localHistory` array to choose items to compare instead of the `allHistory` array. [1]: https://github.com/github/vscode-codeql/blob/bf1e3c10dbf23480d0e0121fe1990e8d07b707c1/extensions/ql-vscode/src/query-history.ts#L1311-L1314
We're adding both remote query history items and variant analysis history items to the query history. We've introduced a little method to shuffle the query history list before we run our tests so that we don't accidentally write tests that depend on a fixed order. The query history now has increased test coverage for: - handling an item being clicked - removing and selecting the next item in query history - handling single / multi selection - showing the item results While we're here we're also: 1. Adding a factory to generate variant analysis history items 2. Providing all fields for remote query history items and ordering them according to their type definition order. At least one field (`queryId`) was missing from our factory, which we will need to make the tests work with remote queries.
I don't know why there are two.
Instead of having them dangle around.
And rename original `getChildren` describe block to `sorting` since that's what the tests are checking.
855f6b5 to
5ee5a6d
Compare
We've introduced a new `local-query-history-item.ts` factory method [1] which includes a cancellation token. The factory will need to import the CancellationTokenSource from `vscode`. We already had a factory method but it didn't quite map with the setup we needed. For example we need to call `.completeQuery` rather than providing a dummy `completedQuery` object. The previous factory method was used in the tests for `query-history-info.test.ts`. Because that factory omitted the cancellation token, we could get away with having these tests in the `tests/pure-tests` folder. With the addition of the second factory method, the tests for `query-history-info` blow up because they can't find `vscode`. Now that we need to add more fields to local query history items, it's becoming clearer that these `query-history-info` tests should live next to the `query-history` tests in `vscode-tests/no-workspace`. Granted, in an ideal situation we'd only have one factory method to generate a local query history item, but combining these two methods is actually quite painful. So for now let's at least have the query history tests next to each other and appease Typescript.
In [1] we changed our factory methods to actually use QueryStatus when creating remote query & variant analysis history items. Previously we were just setting the value to `in progress`... ... which made the tests for history-item-label-provider.test.ts pass... ... but that value did not reflect reality ... What we actually need to do is introduce a method to map different query statuses to human readable strings, e.g. QueryStatus.InProgress becomes 'in progress' [1]: 4b9db6a#diff-217b085c45cd008d938c3da4714b5782db6ad31438b27b07e969254feec8298aL28
We were expecting all three types to behave the same when clicked / double clicked. In fact local & remote queries only allow you to open the results view when they're complete, while variant analyses always allow you to open the results view no matter what their status. Let's break down these tests per history item type and test the expected behaviour more granularly. NB: I tried moving some of the setup in beforeEach blocks, but alas queryHistoryManager can be undefined so rather than adding `?` to every method call, I'm just gonna leave the setup inside the tests. In an ideal world, we'd stop declaring `queryHistoryManager` as `undefined`: ``` let queryHistoryManager: QueryHistoryManager | undefined; ``` Baby steps!
5ee5a6d to
a1daa91
Compare
Paired with @robertbrignull on debugging why having all types of query history items isn't playing nicely when we try to remove an item. We've tracked down the issue it the handleRemoveHistoryItem method not correctly setting the `current` item after a deletion. However, it's unclear whether the test setup is to blame or this is a real bug. I'm going to leave the tests for `handleRemoveHistoryItem` to test just local queries for now (as they were originally) and will come back to this in a different PR.
0cc631d to
5a40159
Compare
koesie10
reviewed
Nov 2, 2022
To produce valid history items and have them blow up when we need to add new fields.
da61b9e to
841c66c
Compare
koesie10
approved these changes
Nov 2, 2022
3 tasks
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
2 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.
This PR holds part of the clean-up work to allow us to write more tests for the
QueryHistoryManagerandHistoryTreeDataProviderby re-organising tests to make it clear what they're checking and removingsome duplication in the process. This unfortunately results in a lot of noisy diffs as I shuffle things into
describeblocks.We're also changing part of these tests to work with
RemoteQueryHistoryItems andVariantAnalysisHistoryItems, instead of just testing local query history items.The query history now has increased test coverage in terms of how items of different types interact
together, specifically:
removing and selecting an itemthis has been reverted, see 5a40159 for detailsIn a future PR we'll also look at:
Disclaimer: I don't think these tests are perfect and there's definitely still some duplication between the
tests in
query-history.test.tsand the individual history item test files, but they're a step in the rightdirection as they untangle some things that are annoying to deal with in terms of writing/changing
query history tests.
Checklist
ready-for-doc-reviewlabel there.