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

Update firestore filtering #480

Merged
merged 1 commit into from
Dec 5, 2023
Merged

Update firestore filtering #480

merged 1 commit into from
Dec 5, 2023

Conversation

danghai
Copy link
Collaborator

@danghai danghai commented Nov 30, 2023

Fix #431

@danghai
Copy link
Collaborator Author

danghai commented Nov 30, 2023

@spbnick , I am not sure why check_deployment is still skipped? It is because of failing flake8?
image

@spbnick
Copy link
Collaborator

spbnick commented Nov 30, 2023

Yeah, it's only executed when everything else has passed, IIRC, to save resources.

@danghai
Copy link
Collaborator Author

danghai commented Nov 30, 2023

Do you have the command that I can run the test locally? I mean this one:

  File "/home/runner/work/kcidb/kcidb/kcidb/test_monitor.py", line 841, in test_email_generated
    assert len(remaining_obj_types) == 0, \
AssertionError: Missing ***'build', 'test', 'checkout', 'revision'*** emails for <class 'kcidb_io.schema.v03_00.Version'>
assert 4 == 0
 +  where 4 = len(***'build', 'checkout', 'revision', 'test'***)

I change test_wipe_main, it passes this testcase; However it is failing test_monitor. I am not sure why
image

@danghai
Copy link
Collaborator Author

danghai commented Dec 1, 2023

Oh it passes now after re-run :D. @spbnick could you review my PR? It resolves the warning.

@spbnick
Copy link
Collaborator

spbnick commented Dec 3, 2023

Do you have the command that I can run the test locally? I mean this one:

Most of the tests you can run with just pytest. However, some of the tests require access to a deployment, which requires you creating a Google Cloud project (you can have six months of free trial if you provide a credit card, IIRC).

The general workflow looks something like this:

./cloud deploy "<gcp_project>" "<namespace>" 1 -v --smtp-mocked --test --heavy-asserts # Create a deployment in your project
./cloud shell "<gcp_project>" "<namespace>" 1 --smtp-mocked --test --heavy-asserts # Start a shell with environment variables pointing to the deployment
export KCIDB_DEPLOYMENT="This deployment is empty" # Only do this with *actual* empty deployments, needed for the monitor test
pytest # Run the tests, repeat as many times as necessary, with whatever options you need
exit # Exit the shell
./cloud withdraw "<gcp_project>" "<namespace>" 1 -v --smtp-mocked --test # Withdraw (destroy) the deployment (leaving some shared objects behind)

Where <gcp_project> is the name of the Google Cloud Project you have created. And <namespace> is a "namespace" - a prefix for the names of (almost) all created objects, that lets you distinguish deployments within a single project. The latter can be an empty string.

There's of course much more to it, but you can start with these, and modify, after reading the corresponding --help output and the docs, where there are any.

@spbnick
Copy link
Collaborator

spbnick commented Dec 3, 2023

Oh it passes now after re-run :D. @spbnick could you review my PR? It resolves the warning.

Ah great! However, in general the test shouldn't be flaky and this warrants investigation. Don't rerun it if you see it again, let me see what's going on first, please.

The change looks good, but the commit message could be better. As such it reads like "Change firestore filtering", and doesn't say much. Could you make it clearer? Even better if it has a reference to the Google documentation covering this change in the API.

Thank you 🙇‍♂️

Fix: #431
UserWarning in build log: Detected filter using positional arguments.
Prefer using the 'filter' keyword argument instead
The filter argument takes BaseFilter class which is an abstract class
and it is implemented in FieldFilter. This class can be imported from
google.cloud.firestore_v1.base_query
Reference: googleapis/python-firestore#705
	   GoogleCloudPlatform/python-docs-samples#10407
@danghai
Copy link
Collaborator Author

danghai commented Dec 3, 2023

@spbnick Thank you. I add more information for the change

Copy link
Collaborator

@spbnick spbnick left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you, Hai!

@spbnick spbnick merged commit c2de292 into main Dec 5, 2023
5 checks passed
@spbnick spbnick deleted the haidang/test branch December 5, 2023 09:21
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.

Update firestore filtering
2 participants