Add Firestore internal tests to build_testapps, and add iOS and Android support. #343
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
on various platforms for various reasons. b/183294303 tracks re-enabling them.
(Mark the ones that don't work as skipped.)
jonsimantov
added
the
tests-requested: quick
github-actions
bot
added
tests: in-progress
firebase-workflow-trigger
bot
removed
the
tests: in-progress
jonsimantov
added
the
tests-requested: quick
github-actions
bot
added
tests: in-progress
firebase-workflow-trigger
bot
removed
the
tests: in-progress
Make sure the internal integration test includes the external one.
firebase-workflow-trigger
bot
removed
the
tests: in-progress
jonsimantov
added
the
tests-requested: quick
github-actions
bot
added
tests: in-progress
firebase-workflow-trigger
bot
removed
the
tests: in-progress
DellaBitta
reviewed
Mar 24, 2021
| IMPORTANT: This file is used by both the regular and the internal Firestore | ||
| integration tests, and needs to be present and identical in both. | ||
| Please sure that any changes to this file are reflected in both of its locations: |
| # Firestore's internal integration test requires absl on Android, | ||
| # so download it now. | ||
| set(ABSEIL_CPP_ROOT ${CMAKE_CURRENT_LIST_DIR}/external/abseil-cpp/src/abseil-cpp) | ||
| if (NOT EXISTS ${ABSEIL_CPP_ROOT}/absl/base/attributes.h) |
Contributor
There was a problem hiding this comment.
Can this be done on just a directory so that it's less fragile?
|
|
||
| constexpr char kClassName[] = | ||
| PROGUARD_KEEP_CLASS "com/google/android/gms/tasks/CancellationTokenSource"; | ||
| "com/google/android/gms/tasks/CancellationTokenSource"; |
Contributor
There was a problem hiding this comment.
Leaving this note for your review to ensure this change was intended and proguard isn't required anymore.
Contributor
Author
There was a problem hiding this comment.
Yes, PROGUARD_KEEP_CLASS is only Proguarding classes referenced by our SDK, not by the testapp itself. I moved those to the proguard.pro in the testapp.
| #include "firebase/firestore/firestore_errors.h" | ||
| #include "app_framework.h" | ||
| #include "firebase_test_framework.h" | ||
|
|
Contributor
There was a problem hiding this comment.
nit: shouldn't these includes be alphabetized?
| #include "android/firestore_integration_test_android.h" | ||
| #include "gmock/gmock.h" | ||
| #include "gtest/gtest.h" | ||
| #include "firebase_test_framework.h" |
Contributor
There was a problem hiding this comment.
nit: and alphabetize here.
Edit: Oh, are these additions we're making or changes we're inheriting from the Firestore repo?
Contributor
Author
There was a problem hiding this comment.
firebase_test_framework and app_framework are additions (so that we can run their tests in our own test framework). All other includes are inherited. I'll hold off on alphabetizing these files until we can set up code formatting and alphabetize all.
| @entitlement_path = entitlement_path | ||
| end | ||
| opts.on('-i', '--XCodeCPP.include [include_path]', | ||
| 'Path to additional header search files (optional)') do |include_path| |
Contributor
There was a problem hiding this comment.
Nit: this is confusing. Should it be like above, "Path to additional include files (optional)" ?
jonsimantov
added
the
tests-requested: quick
github-actions
bot
added
tests: in-progress
❌ Integration test FAILEDRequested by @jonsimantov on commit 2015cd6
|
DellaBitta
approved these changes
Mar 24, 2021
jonsimantov
removed
tests: failed
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
This PR adds the Firestore internal integration tests to the "Integration Tests" workflow when building against source. Additionally, it adds iOS and Android support for those tests. (A number of tests were modified or skipped due to issues; these will be fixed in a future PR.)
Removes tests from the old location in firestore/src/tests. If any of these integration tests can actually just be run as unit tests (e.g. they don't talk to the actual Firestore backend), they can be moved back into firestore/src/tests at some point in the future.
Increased test timeout from 5 minutes to 15 minutes across the board, since Firestore tests take much longer.
build_testapps.py
Now will build the internal integration test instead of the regular one, if there is one available and we are building against the source SDK. (The packaged SDK cannot test the internal APIs, so will only test the regular test.)
Because we build and run the internal test INSTEAD of the regular test, the internal test duplicates the regular test's main source file in addition to the bevy of additional tests it runs. This ensures that those tests still pass even when testing against source.
Android support
Built + tested on Android device. Several tests have issues, probably due to JNI and threads (the tests now run in a background thread). Those tests have been marked as skipped.
On Android, because we don't compile the Firestore core library, we bring in absl manually, as the tests themselves require absl.
iOS support
Built + tested on iOS device. Several tests (notably transaction_extra_test) time out on iOS and have been skipped.
A few changes are required to build these tests, in particular the XCode project tool needs to add the SDK directory to the header search paths so that the tests can include internal SDK headers.
Windows
A number of fixes for paths being too long and preprocessor issues.