Conversation
extensions/ql-vscode/test/vscode-tests/no-workspace/databases/local-databases-ui.test.ts
Outdated
Show resolved
Hide resolved
02ccd60 to
bfc102a
Compare
| const selectExistingDatabaseSpy = jest | ||
| .spyOn(databaseUI as any, "selectExistingDatabase") | ||
| .mockResolvedValue(undefined); | ||
|
|
||
| const importNewDatabaseSpy = jest | ||
| .spyOn(databaseUI as any, "importNewDatabase") | ||
| .mockResolvedValue(undefined); |
There was a problem hiding this comment.
Slightly tweaked the tests so that I didn't have to make all the methods public 🤫 This meant I had to use as any here to spy on these methods, but that's probably better?
There was a problem hiding this comment.
Hmm so we're mocking out methods on the object we're testing? That seems a bit odd. Doesn't it means that we're not actually testing them?
There was a problem hiding this comment.
Yeah, I haven't added tests for selectExistingDatabase and importNewDatabase in this PR, just for the overall promptForDatabase dropdown 👀
Happy to have a go now if that makes more sense? Though I was also planning to open a follow-up issue to improve the feature + tests generally 💅🏽
There was a problem hiding this comment.
It's a little bit odd to me that we're testing a function that calls other functions but those other functions are being mocked. You'd normally mock out dependencies etc. but have real implementations of the component that is being tested.
But what you have is a step forward so I'm happy for us to do some further improvements later on.
|
Thanks for the patient reviewing! Finally got round to looking at tests in a bit more detail 🔎 I haven't tested all the possible options, and I'm still mocking some methods (e.g. I didn't have access to a real |
Follow-up to #3214 to add a basic test of the new "Select database" prompt.
I plan to open a new issue for more comprehensive tests and to improve the feature generally, so this is just a starting point and to get advice on whether this is a sensible way to test...
(Example follow-up work: we could probably filter any existing DBs by language, check that we actually have existing DBs, test those different cases...)
Checklist
N/A - test change only
ready-for-doc-reviewlabel there.