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.

Sign up for GitHub

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

Jump to bottom

Firestore Aggregate Count Implementation and Tests #659

Merged
merged 5 commits into from
Apr 6, 2023
Merged

Firestore Aggregate Count Implementation and Tests #659

merged 5 commits into from
Apr 6, 2023

Conversation

tom-andersen
Copy link
Contributor

No description provided.

@tom-andersen tom-andersen requested a review from dconeybe March 23, 2023 17:22
@dconeybe dconeybe added the tests-requested: quick Trigger a quick set of integration tests. label Mar 23, 2023
dconeybe
dconeybe requested changes Mar 29, 2023
View reviewed changes
Copy link
Contributor

@dconeybe dconeybe left a comment

Choose a reason for hiding this comment

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

This basically all looks good to me. Nice job! Just a few very minor nits. Also, once the C++ SDK is merged then we should update this PR with the main branch and run the integration tests by adding the "tests requested: quick" label.

firestore/src/AggregateQuerySnapshot.cs Outdated Show resolved Hide resolved
firestore/src/AggregateQuery.cs Show resolved Hide resolved
firestore/src/AggregateQuerySnapshot.cs Show resolved Hide resolved
dconeybe
dconeybe reviewed Mar 29, 2023
View reviewed changes
firestore/CMakeLists.txt Outdated Show resolved Hide resolved
@dconeybe dconeybe assigned tom-andersen and unassigned dconeybe Mar 29, 2023
@tom-andersen tom-andersen requested a review from dconeybe April 5, 2023 14:30
dconeybe
dconeybe requested changes Apr 5, 2023
View reviewed changes
docs/readme.md Outdated Show resolved Hide resolved
docs/readme.md Outdated Show resolved Hide resolved
firestore/src/AggregateQuery.cs Show resolved Hide resolved
firestore/src/AggregateQuerySnapshot.cs Show resolved Hide resolved
firestore/src/AggregateQuerySnapshot.cs Outdated Show resolved Hide resolved
firestore/src/AggregateSource.cs Outdated Show resolved Hide resolved
@tom-andersen tom-andersen requested a review from dconeybe April 5, 2023 15:42
@tom-andersen tom-andersen requested a review from ehsannas April 5, 2023 16:39
@dconeybe
Copy link
Contributor

dconeybe commented Apr 5, 2023

Everything LGTM. I'll give formal approval once the integration tests run and pass.

@tom-andersen I had thought that adding the "Tests Requested: Quick" label would trigger the integration tests; however, that appears to have had no effect at all. Can you check with @cherylEnkidu or the FPL team to see how to trigger the integration tests?

@dconeybe dconeybe removed their assignment Apr 5, 2023
@tom-andersen tom-andersen added the tests-requested: full Trigger a FULL set of integration tests (uses expanded test matrix). label Apr 5, 2023
@tom-andersen tom-andersen enabled auto-merge (squash) April 5, 2023 18:37
@github-actions github-actions bot added tests: in-progress This PR's integration tests are in progress. and removed tests-requested: full Trigger a FULL set of integration tests (uses expanded test matrix). tests-requested: quick Trigger a quick set of integration tests. labels Apr 5, 2023
@github-actions
Copy link

github-actions bot commented Apr 5, 2023 •
edited
Loading

❌  Integration test FAILED

Requested by @firebase-workflow-trigger[bot] on commit bac6eab
Last updated: Thu Apr 6 11:18 PDT 2023
View integration test log & download artifacts

Failures Configs
messaging [TEST] [ERROR] [2020] [1/2 Build OS(s): macos] [1/6 Platform(s): iOS] [1/3 Test Device(s): ios_target]

@github-actions github-actions bot added the tests: failed This PR's integration tests failed. label Apr 5, 2023
@firebase-workflow-trigger firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Apr 5, 2023
@tom-andersen tom-andersen merged commit b4c130d into main Apr 6, 2023
@github-actions github-actions bot added tests: in-progress This PR's integration tests are in progress. tests: failed This PR's integration tests failed. and removed tests: failed This PR's integration tests failed. labels Apr 6, 2023
@firebase-workflow-trigger firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Apr 6, 2023
@firebase firebase locked and limited conversation to collaborators May 7, 2023
@a-maurice a-maurice deleted the tomandersen/count branch May 8, 2024 19:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@dconeybe dconeybe dconeybe approved these changes

@ehsannas ehsannas Awaiting requested review from ehsannas

Assignees

@tom-andersen tom-andersen

@dconeybe dconeybe

Labels
tests: failed This PR's integration tests failed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tom-andersen @dconeybe