Skip to content

feat: Multi db test parametrization#436

Merged
m-strzelczyk merged 14 commits intopreview-multidbfrom
feat-multi-db-test-parametrization
Jun 21, 2023
Merged

feat: Multi db test parametrization#436
m-strzelczyk merged 14 commits intopreview-multidbfrom
feat-multi-db-test-parametrization

Conversation

@Mariatta
Copy link
Contributor

@Mariatta Mariatta commented May 9, 2023 •
edited
Loading

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:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

This PR adds changes on top of #398, which was not yet merged to main.

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.
  • If the database is default, the request will not pass any database_id values
  • If the database name is specified, the request will pass a database_id field

Fixes #<issue_number_goes_here> 🦕

@Mariatta Mariatta requested a review from a team as a code owner May 9, 2023 05:19
@Mariatta Mariatta requested review from a team May 9, 2023 05:19
@product-auto-label product-auto-label bot added size: xl Pull request size is extra large. api: datastore Issues related to the googleapis/python-datastore API. labels May 9, 2023
@Mariatta
Copy link
Contributor Author

Mariatta commented May 9, 2023

The systems tests are failing because we don't have the system-tests-named-db on the project used by CI.

Once the database created, populated with data, and indexes created, the tests should pass.

@kolea2 kolea2 added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label May 9, 2023
@Mariatta Mariatta changed the base branch from main to multi-db May 9, 2023 20:11
@Mariatta Mariatta requested a review from a team as a code owner May 9, 2023 20:11
@Mariatta Mariatta requested review from atulep and removed request for a team May 9, 2023 20:11
@Mariatta Mariatta changed the base branch from multi-db to main May 9, 2023 20:11
@Mariatta Mariatta mentioned this pull request May 9, 2023
Merged
@Mariatta Mariatta changed the base branch from main to preview-multidb May 9, 2023 20:15
Mariatta added 3 commits May 9, 2023 13:25
@Mariatta
Feat: Parametrize the tests for multidb support
bd88cbf 
Remove "database" argument from Query and AggregationQuery constructors.
Use the "database" from the client instead.
Once set in the client, the "database" will be used throughout and cannot be re-set.

Parametrize the tests where-ever clients are used.

Use the `system-tests-named-db` in the system test.
@Mariatta
Add test case for when parent database name != child database name
1b7f92c
@Mariatta
Update owlbot, removing the named db parameter
544dab7
@Mariatta Mariatta force-pushed the feat-multi-db-test-parametrization branch from 513e772 to 544dab7 Compare May 9, 2023 20:27
@Mariatta
Copy link
Contributor Author

Mariatta commented May 9, 2023

This PR is now rebased against preview-multidb branch.

@Mariatta
Copy link
Contributor Author

Mariatta commented May 9, 2023

Systems test are now passing.

@rwhogg rwhogg mentioned this pull request May 24, 2023
Merged
@kolea2 kolea2 removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jun 6, 2023
@vishwarajanand
Copy link
Contributor

vishwarajanand commented Jun 13, 2023

I've updated these PRs and got the tests green. Seeking a go ahead for merging this.

rwhogg
rwhogg reviewed Jun 14, 2023
View reviewed changes
@meredithslota meredithslota removed the request for review from atulep June 14, 2023 17:36
@meredithslota
Copy link
Contributor

meredithslota commented Jun 14, 2023

Removed a review request from Aza due to team transfer, but still seeking approval from a Python person.

m-strzelczyk
m-strzelczyk requested changes Jun 20, 2023
View reviewed changes
@parthea parthea requested a review from m-strzelczyk June 20, 2023 20:04
@m-strzelczyk m-strzelczyk merged commit 5499761 into preview-multidb Jun 21, 2023
@m-strzelczyk m-strzelczyk deleted the feat-multi-db-test-parametrization branch June 21, 2023 10:34
m-strzelczyk pushed a commit that referenced this pull request Jun 21, 2023
@Mariatta @gcf-owl-bot
@vishwarajanand @meredithslota
feat: named database support (#439)
abf0060 
* feat: named database support (#398)

* feat: Add named database support

* test: Use named db in system tests

* 🦉 Updates from OwlBot post-processor

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

* Handle the case when client doesn't have database property

* fix: add custom routing headers

* Fixing tests for easier merge

* fixing code coverage

* addressing pr comments

* feat: Multi db test parametrization (#436)

* Feat: Parametrize the tests for multidb support

Remove "database" argument from Query and AggregationQuery constructors.
Use the "database" from the client instead.
Once set in the client, the "database" will be used throughout and cannot be re-set.

Parametrize the tests where-ever clients are used.

Use the `system-tests-named-db` in the system test.

* Add test case for when parent database name != child database name

* Update owlbot, removing the named db parameter

* Reverted test fixes

* fixing tests

* fix code coverage

* pr suggestion

* address pr comments

---------

Co-authored-by: Vishwaraj Anand <vishwaraj.anand00@gmail.com>

---------

Co-authored-by: Bob "Wombat" Hogg <wombat@rwhogg.site>
Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
Co-authored-by: Vishwaraj Anand <vishwaraj.anand00@gmail.com>
Co-authored-by: meredithslota <meredithslota@google.com>
@kolea2 kolea2 mentioned this pull request Jun 21, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@parthea parthea parthea approved these changes

@m-strzelczyk m-strzelczyk m-strzelczyk approved these changes

+2 more reviewers

@rwhogg rwhogg rwhogg approved these changes

@vishwarajanand vishwarajanand vishwarajanand left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

api: datastore Issues related to the googleapis/python-datastore API. size: xl Pull request size is extra large.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@Mariatta @vishwarajanand @meredithslota @rwhogg @parthea @m-strzelczyk @kolea2