Skip to content

operator== and operator!= for SnapshotMetadata and Settings #596

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
Merged
merged 7 commits into from
Aug 11, 2021
Merged

operator== and operator!= for SnapshotMetadata and Settings #596

merged 7 commits into from
Aug 11, 2021

Conversation

ehsannas
Copy link
Contributor

No description provided.

@google-cla google-cla bot added the cla: yes label Aug 10, 2021
@ehsannas ehsannas requested a review from var-const August 10, 2021 19:36
@ehsannas ehsannas assigned ehsannas and var-const and unassigned ehsannas Aug 10, 2021
@ehsannas ehsannas changed the title Add operator== and operator!= for SnapshotMetadata. Add operator== and operator!= for SnapshotMetadata and Settings Aug 10, 2021
@ehsannas ehsannas changed the title Add operator== and operator!= for SnapshotMetadata and Settings operator== and operator!= for SnapshotMetadata and Settings Aug 10, 2021
var-const
var-const suggested changes Aug 11, 2021
View reviewed changes
Copy link
Contributor

@var-const var-const left a comment

Choose a reason for hiding this comment

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

Overall looks good, just some nits.

@var-const var-const assigned ehsannas and unassigned var-const Aug 11, 2021
@var-const
Copy link
Contributor

Also, can you please add a release note?

@var-const
Copy link
Contributor

@ehsannas Please add a release note.

var-const
var-const reviewed Aug 11, 2021
View reviewed changes
Copy link
Contributor

@var-const var-const left a comment

Choose a reason for hiding this comment

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

Almost LGTM (the main remaining thing is to add a release note).

ehsannas
ehsannas commented Aug 11, 2021
View reviewed changes
Copy link
Contributor Author

@ehsannas ehsannas left a comment

Choose a reason for hiding this comment

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

Done. Also added release notes.

@var-const
Copy link
Contributor

@ehsannas Sorry, I missed the commit with the release note -- please disregard my comment.

var-const
var-const previously approved these changes Aug 11, 2021
View reviewed changes
@var-const var-const assigned ehsannas and unassigned var-const Aug 11, 2021
@ehsannas ehsannas added the tests-requested: quick Trigger a quick set of integration tests. label Aug 11, 2021
@github-actions github-actions bot removed the tests-requested: quick Trigger a quick set of integration tests. label Aug 11, 2021
@github-actions github-actions bot added the tests: in-progress This PR's integration tests are in progress. label Aug 11, 2021
@github-actions
Copy link

github-actions bot commented Aug 11, 2021 •
edited
Loading

❌  Integration test FAILED

Requested by @ehsannas on commit 21cb5de
Last updated: Wed Aug 11 15:03 PDT 2021
View integration test log & download artifacts

Failures Configs
missing_log [TEST] [ERROR] [iOS] [macos] [simulator_target]
firestore [TEST] [FAILURE] [Android] [windows] [emulator_target]
(1 failed tests)  FirestoreIntegrationTest.TestCanQueueWritesWhileOffline
messaging [TEST] [ERROR] [Android] [All os] [android_target]

Add flaky tests to go/fpl-cpp-flake-tracker

@github-actions github-actions bot added the tests: failed This PR's integration tests failed. label Aug 11, 2021
@firebase-workflow-trigger firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Aug 11, 2021
@ehsannas
Copy link
Contributor Author

There is a legit test failure for Android:

All tests in the same test suite must use the same test fixture
class, so mixing TEST_F and TEST in the same test suite is
illegal.  In test suite SettingsTest,
test ConverterBoolsAllTrue is defined using TEST_F but
test Equality is defined using TEST.  You probably
want to change the TEST to TEST_F or move it to another test
case.

I'll rename the Android-specific settings test from SettingsTest to SettingsTestAndroid.

@github-actions github-actions bot dismissed var-const’s stale review August 11, 2021 16:34

🍞 Dismissed stale approval on external PR.

@ehsannas ehsannas added tests-requested: quick Trigger a quick set of integration tests. and removed tests: failed This PR's integration tests failed. labels Aug 11, 2021
@github-actions github-actions bot added tests: in-progress This PR's integration tests are in progress. tests: succeeded This PR's integration tests succeeded. and removed tests-requested: quick Trigger a quick set of integration tests. labels Aug 11, 2021
@firebase-workflow-trigger firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Aug 11, 2021
@ehsannas ehsannas requested a review from var-const August 11, 2021 17:59
@ehsannas ehsannas assigned var-const and unassigned ehsannas Aug 11, 2021
@ehsannas
Copy link
Contributor Author

Integration tests pass now. PTAL @var-const

@var-const var-const assigned ehsannas and unassigned var-const Aug 11, 2021
@ehsannas ehsannas merged commit 21cb5de into main Aug 11, 2021
@ehsannas ehsannas deleted the ehsann-add-operator-equals branch August 11, 2021 20:02
@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: succeeded This PR's integration tests succeeded. labels Aug 11, 2021
@firebase-workflow-trigger firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Aug 11, 2021
@firebase firebase locked and limited conversation to collaborators Sep 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@var-const var-const var-const approved these changes

Assignees

@ehsannas ehsannas

Labels
api: firestore cla: yes 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
@ehsannas @var-const