Skip to content

Make C++ validation tests run on all platforms #511

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 25 commits into from
Jul 23, 2021
Merged

Make C++ validation tests run on all platforms #511

merged 25 commits into from
Jul 23, 2021

Conversation

var-const
Copy link
Contributor

@var-const var-const commented Jul 6, 2021 •
edited
Loading

Also:

  • updated the GoogleTest version (to be able to use the Throws matcher);
  • enabled exceptions in Firestore integration tests (otherwise validation tests would be ignored);
  • add some missing validation (see the linked bug for remaining gaps);
  • replace all usage of FIREBASE_ASSERT macros with SIMPLE_HARD_ASSERT (which throws an exception on Unity / crash in C++).

See b/136012313 for more context.

@google-cla google-cla bot added the cla: yes label Jul 6, 2021
@var-const var-const requested a review from wu-hui July 6, 2021 02:03
@var-const var-const added api: firestore tests-requested: quick Trigger a quick set of integration tests. labels Jul 6, 2021
@github-actions github-actions bot added tests: in-progress This PR's integration tests are in progress. and removed tests-requested: quick Trigger a quick set of integration tests. labels Jul 6, 2021
@github-actions
Copy link

github-actions bot commented Jul 6, 2021 •
edited
Loading

❌  Integration test FAILED

Requested by @var-const on commit 890defd
Last updated: Fri Jul 23 18:23 PDT 2021
View integration test log & download artifacts

Failures Configs
firestore [TEST] [FAILURE] [Android] [macos] [android_latest]
(2 failed tests)  IncrementTwiceInABatch
  MergeOnNonExistingDocumentWithIncrement

@github-actions github-actions bot added the tests: failed This PR's integration tests failed. label Jul 6, 2021
@firebase-workflow-trigger firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Jul 6, 2021
@var-const var-const requested review from dconeybe and removed request for wu-hui July 10, 2021 02:05
@var-const var-const assigned dconeybe and unassigned wu-hui Jul 10, 2021
dconeybe
dconeybe requested changes Jul 12, 2021
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.

I think you should add a change log entry, since there are externally-visible behavior changes being made by this PR.

@dconeybe dconeybe assigned var-const and unassigned dconeybe Jul 12, 2021
@var-const var-const assigned dconeybe and unassigned var-const Jul 13, 2021
@dconeybe dconeybe assigned var-const and unassigned dconeybe Jul 13, 2021
@github-actions github-actions bot added tests: in-progress This PR's integration tests are in progress. and removed tests-requested: quick Trigger a quick set of integration tests. labels Jul 23, 2021
@var-const var-const added tests-requested: full Trigger a FULL set of integration tests (uses expanded test matrix). and removed tests: in-progress This PR's integration tests are in progress. labels Jul 23, 2021
@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). labels Jul 23, 2021
@var-const var-const added tests-requested: full Trigger a FULL set of integration tests (uses expanded test matrix). and removed tests: in-progress This PR's integration tests are in progress. labels Jul 23, 2021
@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-requested: full Trigger a FULL set of integration tests (uses expanded test matrix). labels Jul 23, 2021
@firebase-workflow-trigger firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Jul 23, 2021
@var-const var-const added tests-requested: full Trigger a FULL set of integration tests (uses expanded test matrix). and removed tests: failed This PR's integration tests failed. labels Jul 23, 2021
@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-requested: full Trigger a FULL set of integration tests (uses expanded test matrix). labels Jul 23, 2021
@firebase-workflow-trigger firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Jul 23, 2021
jonsimantov
jonsimantov approved these changes Jul 23, 2021
View reviewed changes
Copy link
Contributor

@jonsimantov jonsimantov left a comment

Choose a reason for hiding this comment

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

LGTM for the googletest changes. Deferring to Denver's review for the Firestore-specific test changes.

@var-const var-const added tests: succeeded This PR's integration tests succeeded. and removed tests: failed This PR's integration tests failed. labels Jul 23, 2021
@var-const var-const merged commit 890defd into main Jul 23, 2021
@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 Jul 23, 2021
@firebase-workflow-trigger firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Jul 24, 2021
@firebase firebase locked and limited conversation to collaborators Aug 23, 2021
@jonsimantov jonsimantov deleted the varconst/validation-tests branch December 22, 2021 22:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jonsimantov jonsimantov jonsimantov approved these changes

@dconeybe dconeybe dconeybe approved these changes

Assignees

@var-const var-const

@jonsimantov jonsimantov

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.
5 participants
@var-const @jonsimantov @dconeybe @vimanyu @wu-hui