feat: Multi db test parametrization - #717
Conversation
The changes are: - adding test parametrization wherever the client is used. It will run the test against the default db, and a named db named db can only be passed to the Client, and the underlying queries and aggregation queries will retrieve the database id from the Client.
…m/googleapis/python-firestore into feat-multidb-test-parametrization
kolea2
left a comment
There was a problem hiding this comment.
I dont think the ITs were run since this is a draft PR - can we convert to ready for review?
| @@ -36,26 +37,42 @@ def _make_credentials(): | |||
|
|
|||
| def _make_default_client(*args, **kwargs): | |||
There was a problem hiding this comment.
it would be great to add one additional test without parameterization testing client initialization with:
- not setting the database
- setting the database
- setting the database to None
and asserting the database is set correctly.
There was a problem hiding this comment.
@kolea2 there are 2 approaches to tackle this:
-
(current - neater approach but tests only client initialization) Adds
test_client_constructor_without_dbwhere DB isn't set and the database is asserted to equalDEFAULT_DATABASE -
(Code become less readable, but tests are more exhaustive) we can add a check in
_make_default_clientto not usedatabaseparam when it's an empty string during the client initialization.
I choose the first approach for readability, but I have no qualms with the second one too. Leaving this comment open for now for Python reviewers.
kolea2
left a comment
There was a problem hiding this comment.
lgtm, will defer to @googleapis/yoshi-python for the remaining review!
The changes are:
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #<issue_number_goes_here> 🦕