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.

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 25 commits into from
Jul 23, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
1a3dd04
Initial commit
var-const Jul 6, 2021
363bc6a
Feedback
var-const Jul 13, 2021
d14545c
Feedback 2
var-const Jul 13, 2021
9367222
Fix typo
var-const Jul 14, 2021
432d8a8
Fixes
var-const Jul 16, 2021
f8eda8c
Fixes
var-const Jul 16, 2021
8643722
Bump the Googletest version to 1.11.0.
var-const Jul 16, 2021
18d35c3
Add a test for when an invalid document is passed to a transaction.
var-const Jul 16, 2021
c739757
Merge branch 'main' into varconst/validation-tests
var-const Jul 17, 2021
bf3cf1e
File format
var-const Jul 17, 2021
4d8aaf9
Merge branch 'varconst/validation-tests' of github.com:firebase/fireb…
var-const Jul 19, 2021
47c5466
Check that the given document reference is valid in Transaction and W…
var-const Jul 19, 2021
8bdf6f6
Fix the reference to a different database test
var-const Jul 20, 2021
dbb9f71
Android fixes
var-const Jul 20, 2021
acf550b
Android fixes wip
var-const Jul 20, 2021
3302a68
Android fixes wip
var-const Jul 20, 2021
3496a05
Fix Android tests
var-const Jul 20, 2021
381fecc
Add readme, remove exceptions check
var-const Jul 20, 2021
f1cd482
Merge branch 'main' into varconst/validation-tests
var-const Jul 20, 2021
d9e0667
Fix cleanup tests.
var-const Jul 20, 2021
6ef578b
Merge branch 'main' into varconst/validation-tests
var-const Jul 22, 2021
0e31cf1
Feedback
var-const Jul 22, 2021
a543436
Merge branch 'main' into varconst/validation-tests
var-const Jul 22, 2021
c23c094
Updated submodule
var-const Jul 23, 2021
7ffbd44
Merge branch 'main' into varconst/validation-tests
var-const Jul 23, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions cmake/external/googletest.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -18,20 +18,20 @@ if(TARGET googletest OR NOT DOWNLOAD_GOOGLETEST)
return()
endif()

set(version 1.10.0)
set(version 1.11.0) # June 2021

ExternalProject_Add(
googletest

DOWNLOAD_DIR ${FIREBASE_DOWNLOAD_DIR}
DOWNLOAD_NAME googletest-${version}.tar.gz
URL https://github.com/google/googletest/archive/release-${version}.tar.gz
URL_HASH SHA256=9dc9157a9a1551ec7a7e43daea9a694a0bb5fb8bec81235d8a1e6ef64c716dcb
URL_HASH SHA256=b4870bf121ff7795ba20d20bcdd8627b8e088f2d1dab299a031c1034eddc93d5

PREFIX ${PROJECT_BINARY_DIR}

CONFIGURE_COMMAND ""
BUILD_COMMAND ""
INSTALL_COMMAND ""
TEST_COMMAND ""
)
)
4 changes: 4 additions & 0 deletions firestore/integration_test_internal/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -378,5 +378,9 @@ add_subdirectory(${FIREBASE_CPP_SDK_DIR} bin/ EXCLUDE_FROM_ALL)
# Note that firebase_app needs to be last in the list.
set(firebase_libs firebase_firestore firebase_auth firebase_app)
set(gtest_libs gtest gmock)
# Validation tests need exceptions to be enabled in order to run. The only
# library that explicitly disables exceptions is App (but it gets propagated
# everywhere), so overriding that effectively reenables exceptions.
target_compile_options(firebase_app PUBLIC -fexceptions)
Copy link
Contributor

Choose a reason for hiding this comment

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

This will actually be nice to have exceptions enabled because it makes it easier to fail a test from a helper method.

https://github.com/google/googletest/blob/master/docs/advanced.md#using-assertions-in-sub-routines

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI This change probably won't be required once #561 is merged. Can you see if you can undo this change if you merge after it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the pointer! I'll give it a try once that PR is merged.

target_link_libraries(${integration_test_target_name} ${firebase_libs}
${gtest_libs} ${ADDITIONAL_LIBS})
17 changes: 10 additions & 7 deletions firestore/integration_test_internal/src/cleanup_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -245,18 +245,18 @@ void ExpectAllMethodsAreNoOps(QuerySnapshot* ptr) {
EXPECT_EQ(ptr->size(), 0);
}

void ExpectAllMethodsAreNoOps(WriteBatch* ptr) {
void ExpectAllMethodsAreNoOps(WriteBatch* ptr, const DocumentReference& doc) {
EXPECT_FALSE(ptr->is_valid());

// `WriteBatch` isn't equality comparable.
ExpectCopyableAndMoveable(ptr);

ptr->Set(DocumentReference(), MapFieldValue());
ptr->Set(doc, MapFieldValue());

ptr->Update(DocumentReference(), MapFieldValue());
ptr->Update(DocumentReference(), MapFieldPathValue());
ptr->Update(doc, MapFieldValue());
ptr->Update(doc, MapFieldPathValue());

ptr->Delete(DocumentReference());
ptr->Delete(doc);

EXPECT_EQ(ptr->Commit(), FailedFuture<void>());
}
Expand Down Expand Up @@ -409,17 +409,20 @@ TEST_F(CleanupTest, QuerySnapshotIsBlankAfterCleanup) {
// case where a user could be accessing a "blank" transaction.

TEST_F(CleanupTest, WriteBatchIsBlankAfterCleanup) {
// Need a valid `DocumentReference` to avoid `WriteBatch` methods throwing.
DocumentReference doc = Document();

{
WriteBatch default_constructed;
SCOPED_TRACE("WriteBatch.DefaultConstructed");
ExpectAllMethodsAreNoOps(&default_constructed);
ExpectAllMethodsAreNoOps(&default_constructed, doc);
}

Firestore* db = TestFirestore();
WriteBatch batch = db->batch();
DeleteFirestore(db);
SCOPED_TRACE("WriteBatch.AfterCleanup");
ExpectAllMethodsAreNoOps(&batch);
ExpectAllMethodsAreNoOps(&batch, doc);
}

} // namespace
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -118,14 +118,19 @@ FirestoreIntegrationTest::FirestoreIntegrationTest() {

Firestore* FirestoreIntegrationTest::TestFirestore(
const std::string& name) const {
return TestFirestoreWithProjectId(name, /*project_id=*/"");
}

Firestore* FirestoreIntegrationTest::TestFirestoreWithProjectId(
const std::string& name, const std::string& project_id) const {
for (const auto& entry : firestores_) {
const FirestoreInfo& firestore_info = entry.second;
if (firestore_info.name() == name) {
return firestore_info.firestore();
}
}

App* app = GetApp(name.c_str());
App* app = GetApp(name.c_str(), project_id);
if (apps_.find(app) == apps_.end()) {
apps_[app] = std::unique_ptr<App>(app);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ FirestoreInternal* CreateTestFirestoreInternal(App* app);
void InitializeFirestore(Firestore* instance);

App* GetApp();
App* GetApp(const char* name);
App* GetApp(const char* name, const std::string& override_project_id);
bool ProcessEvents(int msec);

// Converts a Firestore error code to a human-friendly name. The `error_code`
Expand Down Expand Up @@ -184,6 +184,11 @@ class FirestoreIntegrationTest : public testing::Test {
// if `DeleteApp()` is called with the `App` of the returned `Firestore`.
Firestore* TestFirestore(const std::string& name = kDefaultAppName) const;

// Returns a Firestore instance for an app with the given `name`, associated
// with the database with the given `project_id`.
Firestore* TestFirestoreWithProjectId(const std::string& name,
const std::string& project_id) const;

// Deletes the given `Firestore` instance, which must have been returned by a
// previous invocation of `TestFirestore()`, and removes it from the cache of
// instances returned from `TestFirestore()`. Note that all `Firestore`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ struct TestFriend {
}
};

App* GetApp(const char* name) {
App* GetApp(const char* name, const std::string& override_project_id) {
// TODO(varconst): try to avoid using a real project ID when possible. iOS
// unit tests achieve this by using fake options:
// https://github.com/firebase/firebase-ios-sdk/blob/9a5afbffc17bb63b7bb7f51b9ea9a6a9e1c88a94/Firestore/core/test/firebase/firestore/testutil/app_testing.mm#L29
Expand All @@ -48,16 +48,22 @@ App* GetApp(const char* name) {
App* default_app = App::GetInstance();
SIMPLE_HARD_ASSERT(default_app,
"Cannot create a named app before the default app");

AppOptions options = default_app->options();
if (!override_project_id.empty()) {
options.set_project_id(override_project_id.c_str());
}

#if defined(__ANDROID__)
return App::Create(default_app->options(), name, app_framework::GetJniEnv(),
return App::Create(options, name, app_framework::GetJniEnv(),
app_framework::GetActivity());
#else
return App::Create(default_app->options(), name);
return App::Create(options, name);
#endif // defined(__ANDROID__)
}
}

App* GetApp() { return GetApp(nullptr); }
App* GetApp() { return GetApp(/*name=*/nullptr, /*project_id=*/""); }

FirestoreInternal* CreateTestFirestoreInternal(App* app) {
return TestFriend::CreateTestFirestoreInternal(app);
Expand Down
Loading