Add integration tests of QueryDiscovery#2447
Conversation
|
|
||
| onWatcherDidChangeEvent.fire(workspace.workspaceFolders![0].uri); | ||
|
|
||
| // Wait for refresh to finish |
There was a problem hiding this comment.
I'm not fully happy with this. It appears to work, but I'd like to avoid sleeps like this because they can be a source of flakiness.
I was thinking if we could modify the Discovery class so you can await the completion of the current refresh. It might be a little fiddly, but it could make these tests and QLTestDiscovery tests more robust.
There was a problem hiding this comment.
I'll open an internal issue for this because I want to address it and not forget about it, but also don't want to delay this PR. I do want to continue working on these tests for the next few days and make them better.
shati-patel
left a comment
shati-patel
left a comment
There was a problem hiding this comment.
Just a small comment, but LGTM!
I've made the tests work with the code is it is now, but honestly after this PR I'm considering removing some of the vscode abstractions. So that basically means reverting #2437 and #2431 because they arguably make the code more confusing and testing harder.
Thanks for thinking about this! I don't have a strong opinion on it either way 😅
…es-panel/query-discovery.test.ts Co-authored-by: Shati Patel <42641846+shati-patel@users.noreply.github.com>
I do want to continue working on these tests and make them better and the production code clearer too. So I just want to reassure I don't plan to leave tech debt here if at all possible. 👷🏼 |
2 participants
This PR introduces integration tests of the
QueryDiscoveryclass.Includes one fix for something I discovered while writing the tests. If a workspace folder doesn't include any query files, we probably don't want to create a
FileTreeDirectoryfor it. This does raise some questions about what the panel will look like when there's no queries at all, but I think we can solve that and do it separately after this PR.I've made the tests work with the code is it is now, but honestly after this PR I'm considering removing some of the vscode abstractions. So that basically means reverting #2437 and #2431 because they arguably make the code more confusing and testing harder.
Checklist
ready-for-doc-reviewlabel there.