-
Notifications
You must be signed in to change notification settings - Fork 78
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Support the Count aggregation query #673
Conversation
023213f
to
76a3690
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First pass.
result = count_query.get() | ||
assert len(result) == 1 | ||
for r in result[0]: | ||
assert r.alias == "total" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. Lines up with what I have here https://github.com/googleapis/python-firestore/pull/673/files.
tests/system/test_system.py
Outdated
assert len(result[0]) == 2 | ||
|
||
for r in result[0]: | ||
assert r.alias in ["total", "all"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be more precise to do an assert check here that also verifies the order of "total" and "all" in r.alias? Perhaps two assert statements instead of using a loop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my tests, the order of the result has been inconsistent. It doesn't always return it in the order of "total" and then "all".
I've now adjusted this test to assert that:
- there are two elements returned in the result
- the returned result has two values and they are both unique
- the result contains both aliases: "total" and "all"
Test in transaction
29864f6
to
674d242
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm to me after header fix and PTAL at last comment.
Would like someone from yoshi-python to review as I've only looked from an API perspective. Thanks!
Refactor system test suite to fallback to default creds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm after @googleapis/yoshi-python takes a look as well. Thanks!
credentials = service_account.Credentials.from_service_account_file( | ||
FIRESTORE_CREDS | ||
) | ||
project = FIRESTORE_PROJECT or credentials.project_id | ||
else: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would like someone from @googleapis/yoshi-python to take a look at this specifically
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kolea2 did anyone from @googleapis/yoshi-python review this? I can't tell from the conversation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe @nicain did!
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> 🦕