Skip to content

Bugfix/app cmake exceptions private #561

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

Merged
merged 12 commits into from
Jul 27, 2021
Merged

Conversation

vimanyu
Copy link
Contributor

@vimanyu vimanyu commented Jul 21, 2021

Make the "no exceptions" compiler setting for Firebase app a private dependency so it does not affect the consuming projects.
Some of the unit tests dealing with threads now throw an exception and have been modified to expect such exceptions.

@@ -20,6 +20,17 @@
#include "gmock/gmock.h"
#include "gtest/gtest.h"

#define EXPECT_SYSTEM_ERROR(statement, error_code) \
Copy link
Contributor

Choose a reason for hiding this comment

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

WDYT about naming this something like EXPECT_THROW_WITH_CODE? (just a thought - so it's clearer in the tests that this is specifically testing for a thrown exception.) Or if you think it isn't too wordy, just to use EXPECT_THROW directly with the EXPECT_EQ inside the catch in each of the tests, rather than the #define, since it's all just in this file anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah good point. I wanted to avoid repeating the pattern (EXPECT_THROW with EXPECT_EQ) in multiple places. I could change it to that but it seemed like a lot of code duplication. How about EXPECT_THROW_WITH_ERROR_CODE?

DellaBitta
DellaBitta previously approved these changes Jul 22, 2021
@vimanyu vimanyu added the tests-requested: quick Trigger a quick set of integration tests. label Jul 22, 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 22, 2021
@github-actions
Copy link

github-actions bot commented Jul 22, 2021

❌  Integration test FAILED

Requested by @vimanyu on commit 984f698
Last updated: Mon Jul 26 23:12 PDT 2021
View integration test log & download artifacts

Failures Configs
firestore [TEST] [FAILURE] [Android] [ubuntu] [android_latest]
(1 failed tests)  NumericTransformsTest.DoubleIncrementWithExistingString

@github-actions github-actions bot added the tests: failed This PR's integration tests failed. label Jul 22, 2021
@firebase-workflow-trigger firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Jul 22, 2021
@vimanyu vimanyu added tests-requested: quick Trigger a quick set of integration tests. and removed tests: failed This PR's integration tests failed. labels Jul 22, 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: quick Trigger a quick set of integration tests. labels Jul 22, 2021
@vimanyu vimanyu 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. tests: in-progress This PR's integration tests are in progress. labels Jul 22, 2021
@github-actions github-actions bot removed the tests-requested: full Trigger a FULL set of integration tests (uses expanded test matrix). label Jul 22, 2021
@vimanyu vimanyu added tests-requested: quick Trigger a quick set of integration tests. and removed tests: failed This PR's integration tests failed. labels Jul 26, 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 26, 2021
@vimanyu vimanyu added tests-requested: quick Trigger a quick set of integration tests. and removed tests: in-progress This PR's integration tests are in progress. labels Jul 26, 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 Jul 26, 2021
@firebase-workflow-trigger firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Jul 26, 2021
@vimanyu vimanyu added the tests-requested: quick Trigger a quick set of integration tests. label Jul 27, 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: quick Trigger a quick set of integration tests. tests: succeeded This PR's integration tests succeeded. labels Jul 27, 2021
@firebase-workflow-trigger firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Jul 27, 2021
@vimanyu vimanyu removed the tests: failed This PR's integration tests failed. label Jul 27, 2021
@vimanyu vimanyu merged commit 984f698 into main Jul 27, 2021
@vimanyu vimanyu deleted the bugfix/app-cmake-exceptions-private branch July 27, 2021 03:57
@github-actions github-actions bot added the tests: in-progress This PR's integration tests are in progress. label Jul 27, 2021
@github-actions github-actions bot added the tests: failed This PR's integration tests failed. label Jul 27, 2021
@firebase-workflow-trigger firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Jul 27, 2021
@firebase firebase locked and limited conversation to collaborators Aug 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes tests: failed This PR's integration tests failed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants