Skip to content

Conversation

@yuryfirebolt
Copy link
Contributor

No description provided.

Copy link
Contributor

@stepansergeevitch stepansergeevitch left a comment

Choose a reason for hiding this comment

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

Looks good. Left some comment regarding tests

# get all databases, with name_contains
databases = rm.databases.get_many(name_contains=database_name)
assert len(databases) > 0
assert database_name in {db.name for db in databases}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please also test passing attached_engine_name_eq and attached_engine_name_contains since we know it has engines.
Also, do you think we can test order_by?
We can just see how many databases there are from the first call, and if it's more that one we also try passing order_by

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added tests for attached_engine_name_eq and attached_engine_name_contains. However, I would not do additional tests on order_by, since it looks likes we are re-implementing order_by functionality in the test

Copy link
Contributor

Choose a reason for hiding this comment

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

We can still do some basic checks on order_by without re-implementing the functionality. e.g. checking that the parameter is propagated correctly and is added to the http request.

@yuryfirebolt yuryfirebolt merged commit 21d5542 into main Jan 24, 2022
@yuryfirebolt yuryfirebolt deleted the fix-databases-get-many branch January 24, 2022 09:54
@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

66.7% 66.7% Coverage
0.0% 0.0% Duplication

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.

4 participants