feat: create async interface #61

rafilong · 2020-06-22T23:23:00Z

Add async interface and testers.

Note: tests relying on Collection will fail in this commit

rafilong · 2020-07-06T22:06:41Z

Adding do not merge label - working on a Transaction refactor using multiple inheritance that should reduce clutter. PR coming soon.

rafilong · 2020-07-06T23:06:14Z

Base class refactor now includes #81, which utilizes multiple inheritance to refactor transaction.py.

…nto asyncio

BenWhitehead

Is there a plan to add integration tests in a later PR? There is a lot of mocking going on here and much of it seems to be a little closer to the high level API than I'd expect (but perhaps is necessary due to the way the existing API is implemented).

tests/unit/v1/async/__init__.py

tests/unit/v1/async/test_async_batch.py

tests/unit/v1/async/test_async_client.py

tests/unit/v1/async/test_async_collection.py

tests/unit/v1/async/test_async_document.py

tests/unit/v1/async/test_async_query.py

BenWhitehead · 2020-07-15T17:08:07Z

tests/unit/v1/async/test_async_transaction.py

+            await transaction._begin()
+
+        err_msg = _CANT_BEGIN.format(transaction._id)
+        self.assertEqual(exc_info.exception.args, (err_msg,))


This feels like a hollow assertion since we're constructing the expected result in the exact same why the internal code is. It may be better to actually write out the expected message to verify the internal creation of it does what is desired.

That's a good point. If the construction of the error message was marginally more complex I would agree with you hands down, but I feel as though this is acceptable as it is very close to comparing a defined constant string. @crwilcox do you have any strong opinions about this?

A lot of the exception checking is fragile. We already encountered an issue with this in the microgen conversion. Basically we can use the assertRaisesRegex instead but it isn't much better. We could mock the var so we could see it was called in the impl properly? But again not sure it is better?

TL;DR - ack

tests/unit/v1/async/test_async_transaction.py

google/cloud/firestore_v1/async_client.py

rafilong · 2020-07-15T19:45:53Z

@BenWhitehead my thought is to leave system tests until the async microgen client is integrated, as that way we can test for async functionality. As is all of the classes are stubbed out for async, but the actual networked calls are still blocking. What are your thoughts about doing this?

Also, just to note, most of the current async unit tests are ported from the sync test cases. In that regard, your comments are applicable to both of them. Would it make sense to fix both the async and sync unit tests in this PR, or should I make a new PR to do that?

crwilcox · 2020-07-15T20:47:44Z

Possibly worth noting. This PR does not contain integration of async generated bits. While the surface is async, the underlying code isn't yet.

BenWhitehead · 2020-07-15T20:57:09Z

Sounds good, apologies for not understanding the async integration with generated client hasn't already taken place. I think it's fine to leave adding integration tests until that time.

Regarding changing both the async and sync unit tests I don't think that is probably necessary as long as there are integration tests that will come. The thing that stood out to me is that there weren't integration tests added in this PR.

rafilong · 2020-07-15T21:00:56Z

Sorry I wasn't very clear about integration of async, but the changes to the async and sync tests I was referring to was with regards to the fixes you suggested for the async unit tests. Would it be best to integrate these changes into this PR (like I did with the fixing assert syntax), or bundle them in another PR?

BenWhitehead · 2020-07-15T21:02:13Z

Up to you, either is fine with me.

crwilcox · 2020-07-16T16:43:31Z

It seems any issues are either tracked in issues, or have been addressed. I think this is good to merge.

rafilong added 29 commits June 15, 2020 22:16

feat: add async tests for AsyncClient

f4d89bd

feat: add AsyncClient implementation

d986ed8

feat: add AsyncDocument implementation

54f0289

feat: add AsyncDocument support to AsyncClient

2879c70

feat: add AsyncDocument tests

f9935f7

Note: tests relying on Collection will fail in this commit

feat: add AsyncCollectionReference class

1cc47ad

feat: integrate AsyncCollectionReference

281f804

feat: add async_collection tests

59ae8a6

fix: swap coroutine/function declaration in async_collection

7ab12a0

feat: add async_batch implementation

b700c7b

feat: integrate async_batch

a6a948f

feat: add async_batch tests

e0fb873

feat: add async_query implementation

7e99326

feat: add async_query integration

ec21bdc

feat: add async_query tests

42595bc

fix: AsyncQuery.get async_generator nesting

fb568fd

feat: add async_transaction integration and tests

438bcf3

fix: linter errors

48685ef

feat: refactor async tests to use aiounittest and pytest-asyncio

36e914d

feat: remove duplicate code from async_client

0eec38c

feat: remove duplicate code from async_batch

8110265

feat: remove duplicate code from async_collection

80270de

feat: remove duplicate code from async_document

47199ea

fix: remove unused imports

da04662

fix: remove duplicate test

461f76e

feat: remove duplicate code from async_transaction

0df1c2e

fix: remove unused Python2 compatibility

bac2541

fix: resolve async generator tests

0200323

fix: create mock async generator to get full coverage

8362fc8

rafilong requested a review from crwilcox June 22, 2020 23:23

crwilcox added release-please:force-run To run release-please and removed release-please:force-run To run release-please labels Jul 6, 2020

rafilong added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jul 6, 2020

feat: create Transaction/AsyncTransaction superclass

fd654f1

rafilong removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jul 7, 2020

rafilong added 4 commits July 13, 2020 20:03

Merge branch 'v2-staging' of github.com:googleapis/python-firestore i…

eb0a9c1

…nto asyncio

merge: microgen and v1beta1 changes

a8ee365

feat: add microgen support to async interface

ec5448e

fix: async client copyright date

b6c8380

BenWhitehead reviewed Jul 15, 2020

View reviewed changes

rafilong added 3 commits July 15, 2020 15:21

fix: standardize assert syntax

18bf861

fix: incorrect copyright date

faccb50

fix: incorrect copyright date

1f4ba24

rafilong added 2 commits July 15, 2020 17:13

fix: clarify _sleep assertions in transaction

81b09f8

fix: clarify error in context manager tests

a75ab70

crwilcox approved these changes Jul 16, 2020

View reviewed changes

fix: clarify error in context manager tests

5914bfa

rafilong added automerge Merge the pull request once unit tests and other checks pass. and removed automerge Merge the pull request once unit tests and other checks pass. labels Jul 16, 2020

rafilong merged commit eaba25e into googleapis:v2-staging Jul 16, 2020

rafilong deleted the asyncio branch July 22, 2020 20:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: create async interface #61

feat: create async interface #61

rafilong commented Jun 22, 2020

rafilong commented Jul 6, 2020

rafilong commented Jul 6, 2020

BenWhitehead left a comment

BenWhitehead Jul 15, 2020

rafilong Jul 15, 2020 •

edited

crwilcox Jul 15, 2020

rafilong commented Jul 15, 2020 •

edited

crwilcox commented Jul 15, 2020

BenWhitehead commented Jul 15, 2020

rafilong commented Jul 15, 2020

BenWhitehead commented Jul 15, 2020

crwilcox commented Jul 16, 2020

feat: create async interface #61

feat: create async interface #61

Conversation

rafilong commented Jun 22, 2020

rafilong commented Jul 6, 2020

rafilong commented Jul 6, 2020

BenWhitehead left a comment

Choose a reason for hiding this comment

BenWhitehead Jul 15, 2020

Choose a reason for hiding this comment

rafilong Jul 15, 2020 • edited

Choose a reason for hiding this comment

crwilcox Jul 15, 2020

Choose a reason for hiding this comment

rafilong commented Jul 15, 2020 • edited

crwilcox commented Jul 15, 2020

BenWhitehead commented Jul 15, 2020

rafilong commented Jul 15, 2020

BenWhitehead commented Jul 15, 2020

crwilcox commented Jul 16, 2020

rafilong Jul 15, 2020 •

edited

rafilong commented Jul 15, 2020 •

edited