Skip to content

Add test for "promptForDatabase"#3248

Merged
shati-patel merged 3 commits intomainfrom
shati-patel/select-db-test
Jan 22, 2024
Merged

Add test for "promptForDatabase"#3248
shati-patel merged 3 commits intomainfrom
shati-patel/select-db-test

Conversation

@shati-patel
Copy link
Copy Markdown
Contributor

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

  • 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.

@shati-patel shati-patel force-pushed the shati-patel/select-db-test branch from 02ccd60 to bfc102a Compare January 16, 2024 16:52
@shati-patel shati-patel marked this pull request as ready for review January 16, 2024 17:07
@shati-patel shati-patel requested a review from a team as a code owner January 16, 2024 17:07
Comment on lines +189 to +195
const selectExistingDatabaseSpy = jest
.spyOn(databaseUI as any, "selectExistingDatabase")
.mockResolvedValue(undefined);

const importNewDatabaseSpy = jest
.spyOn(databaseUI as any, "importNewDatabase")
.mockResolvedValue(undefined);
Copy link
Copy Markdown
Contributor Author

@shati-patel shati-patel Jan 17, 2024

Choose a reason for hiding this comment

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

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?

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.

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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 💅🏽

Copy link
Copy Markdown
Contributor

@charisk charisk Jan 17, 2024

Choose a reason for hiding this comment

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

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.

@shati-patel
Copy link
Copy Markdown
Contributor Author

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 DatabaseManager, so I mocked setCurrentDatabaseItem).
But I think this is now more sensible as a test? 😅

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, LGTM!

@shati-patel shati-patel merged commit 83148f6 into main Jan 22, 2024
@shati-patel shati-patel deleted the shati-patel/select-db-test branch January 22, 2024 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants