Skip to content
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

[Firestore] Updated code to run sys tests on emulator. #54

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

sumit-ql
Copy link

Towards [8722].

  1. Updated sys tests code to make it run on emulator.
    5 tests are failing on emulator, all of them are running fine on live firestore service.
    Not sure whether we have all firestore functionality available on emulator or not.

Copy link

@emar-kar emar-kar left a comment

Choose a reason for hiding this comment

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

I don't think it is the correct realization of the requested changes. As I see, you need to modify the Client class and write tests to cover it.

if not os.environ.get("FIRESTORE_APPLICATION_CREDENTIALS", ""):
session.skip("Credentials must be set via environment variable")
if not os.getenv("FIRESTORE_EMULATOR_HOST"):
if not os.environ.get("FIRESTORE_APPLICATION_CREDENTIALS", ""):

Choose a reason for hiding this comment

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

Pay attention! Nox files are generated automatically. Your changes are incorrect and must be rethought.
https://github.com/q-logic/google-cloud-python/blob/f81bf63abf64ab297bff6057b91d1e7cf6576dd6/firestore/noxfile.py#L16-L18

Copy link
Author

@sumit-ql sumit-ql Sep 11, 2019

Choose a reason for hiding this comment

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

Yup, will revert it. Thanks!

Copy link
Author

Choose a reason for hiding this comment

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

Reverted the nox file for now, but i need those changes in nox file.
Should I exclude the noxfile.py from synth tool?

credentials = EmulatorCreds()
project = "emulatorproject"
else:
credentials = service_account.Credentials.from_service_account_file(FIRESTORE_CREDS)

Choose a reason for hiding this comment

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

Lint session fails! Use black to reformat the file.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

@sumit-ql
Copy link
Author

I don't think it is the correct realization of the requested changes. As I see, you need to modify the Client class and write tests to cover it.

Firestore 'Client' class already has the required support for emulator.

@emar-kar emar-kar self-requested a review September 13, 2019 12:16
Copy link

@emar-kar emar-kar left a comment

Choose a reason for hiding this comment

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

Chris answered your questions in the meeting document. Have a look, please! He also created PR to skip those tests.

@@ -126,6 +136,7 @@ def test_create_document_w_subcollection(client, cleanup):
assert sorted(child.id for child in children) == sorted(child_ids)


@pytest.mark.skipif(IN_EMULATOR, reason="Not supported in emulator.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this repeats multiple times, I'd suggest defining a constant, something like
REASON_NOT_SUPPORTED="Not supported in emulator."
and using it throughout the file.

Copy link
Author

Choose a reason for hiding this comment

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

I have put the internal issue tracking number as a reason for all skipped tests as suggested by Chris.

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