Skip to content

leveldb_snappy_test.cc: run the test on iOS as well, and other improvements #896

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 5 commits into from
Apr 28, 2022

Conversation

dconeybe
Copy link
Contributor

Make some improvements to Firestore's leveldb_snappy_test.cc:

  • Run the test on iOS instead of skipping it; on iOS it verifies that snappy support is not available. This is desirable because we don't want snappy support to unexpectedly show up because of forwards-compatibility issues.
  • Fail fast if creating the test LevelDb database fails.
  • Pull in other changes from Enable Snappy compression support in LevelDb in cmake builds firebase-ios-sdk#9596 to bring this test into closer parity with that one.

@dconeybe dconeybe self-assigned this Apr 13, 2022
@dconeybe dconeybe added skip-release-notes Skip release notes check tests-requested: quick Trigger a quick set of integration tests. labels Apr 13, 2022
@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 Apr 13, 2022
@github-actions
Copy link

github-actions bot commented Apr 13, 2022

✅  Integration test succeeded!

Requested by @dconeybe on commit cb4f636
Last updated: Wed Apr 27 20:10 PDT 2022
View integration test log & download artifacts

@github-actions github-actions bot added the tests: succeeded This PR's integration tests succeeded. label Apr 14, 2022
@firebase-workflow-trigger firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Apr 14, 2022
@dconeybe dconeybe requested a review from cherylEnkidu April 14, 2022 03:27
dconeybe added a commit that referenced this pull request Apr 15, 2022
Copy link
Contributor

@cherylEnkidu cherylEnkidu left a comment

Choose a reason for hiding this comment

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

Thank you for your amazing changes! Just have some questions hope you can help me answer them :)

@cherylEnkidu cherylEnkidu removed their assignment Apr 22, 2022
@dconeybe dconeybe added the tests-requested: quick Trigger a quick set of integration tests. label Apr 22, 2022
@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. tests: succeeded This PR's integration tests succeeded. labels Apr 22, 2022
@dconeybe dconeybe requested a review from cherylEnkidu April 22, 2022 19:50
@github-actions github-actions bot added the tests: succeeded This PR's integration tests succeeded. label Apr 22, 2022
@firebase-workflow-trigger firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Apr 22, 2022
cherylEnkidu
cherylEnkidu previously approved these changes Apr 27, 2022
Copy link
Contributor

@cherylEnkidu cherylEnkidu left a comment

Choose a reason for hiding this comment

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

Thank you!

@cherylEnkidu cherylEnkidu removed their assignment Apr 27, 2022
@github-actions github-actions bot dismissed cherylEnkidu’s stale review April 27, 2022 18:00

🍞 Dismissed stale approval on external PR.

@dconeybe dconeybe requested a review from cherylEnkidu April 27, 2022 19:43
@dconeybe dconeybe merged commit cb4f636 into main Apr 28, 2022
@dconeybe dconeybe deleted the dconeybe/SnappyPresenceTestImprovements branch April 28, 2022 00:36
@github-actions github-actions bot added tests: in-progress This PR's integration tests are in progress. and removed tests: succeeded This PR's integration tests succeeded. labels Apr 28, 2022
dconeybe added a commit that referenced this pull request Apr 28, 2022
… iOS as well, and other improvements (#896)

This change improves the leveldb_snappy_test.cc to provide stronger guarantees of expected behavior.
@github-actions github-actions bot added the tests: succeeded This PR's integration tests succeeded. label Apr 28, 2022
@firebase-workflow-trigger firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Apr 28, 2022
jonsimantov added a commit that referenced this pull request May 5, 2022
…port and Firestore (#915)

* Remove the patch to enable Snappy support

* firestore_patch.py: also copy the URL_HASH

* query_main.h: fix build error due to Filter being renamed to FieldFilter.

* user_data_converter_main.cc: fix build error when calling NullValue(), which now just returns a google_firestore_v1_Value instead of a Message<google_firestore_v1_Value>

* Merge in #896 (leveldb_snappy_test.cc: run the test on iOS as well, and other improvements)

* leveldb_snappy_test.cc: re-use the test from the iOS SDK instead of re-writing it.

* leveldb_snappy_test.cc: add a newline at the end of the file.

* query_main.h: add missing #include "Firestore/core/src/core/field_filter.h"

* Set the FIRESTORE_INCLUDE_OBJC cmake cache var to NO to avoid building Objective-C parts of the iOS SDK

This fixes the following build error:

```
FIRFirestore.h:147:20: error: unknown attribute 'swift_async' ignored [-Werror,-Wunknown-attributes]
    __attribute__((swift_async(none)));  // Disable async import due to #9426.
                   ^
1 error generated.
```

which was introduced by firebase/firebase-ios-sdk#9502

* external/pip_requirements.txt: add six

This is an attempt to fix the following build error:

```
Traceback (most recent call last):
  File "Firestore/Protos/tmphmjWeu.py", line 25, in <module>
    import nanopb_generator as nanopb
  File "nanopb/generator/nanopb_generator.py", line 25, in <module>
    import google.protobuf.text_format as text_format
  File "protobuf/python/google/protobuf/text_format.py", line 51, in <module>
    import six
ImportError: No module named six
```

e.g. https://github.com/firebase/firebase-cpp-sdk/runs/6034310890

* Improve FIREBASE_PYTHON_EXECUTABLE cmake cache var and make its usage ubiquitous

* firestore.cmake: bump pinned commit

* Revert "external/pip_requirements.txt: add six" since it didn't fix the problem.

This reverts commit 56553d9.

* firestore.cmake: fix pinned commit

* Revert "Improve FIREBASE_PYTHON_EXECUTABLE cmake cache var and make its usage ubiquitous"

It seemed to cause problems with the build. There is a better way in the iOS SDK anyways.

This reverts commit c6346fd.

* firestore.cmake: update pinned commit

* CMakeLists.txt: Set FIRESTORE_INCLUDE_OBJC as a cache variable so it actually works

* query_main.cc: IsBefore(BoundPosition) -> IsInclusive(BoundPosition)

This fixes several test failures due to incorrect handling of startAfter and endBefore resulting from a change in core::Bound from "before" to "inclusive" in firebase/firebase-ios-sdk#9519

Namely, this fixes the following failure:
```
cursor_test.cc:250: Failure
Expected equality of these values:
  std::vector<std::string>({"c", "f", "b", "e"})
    Which is: { "c", "f", "b", "e" }
  QuerySnapshotToIds(snapshot)
    Which is: { "c", "f" }
[  FAILED  ] FirestoreIntegrationTest.TimestampsCanBePassedToQueriesAsLimits
```

e.g. https://github.com/firebase/firebase-cpp-sdk/runs/6044737005

* firestore.cmake: update pinned commit

* Use staging repo and update to 9.0 podfiles.

Add Swift headers from 9.0.0 zip release.

* Remove streaming support, as it fails with the new Swift SDK.

* Enable Firestore and Admob.

* Remove AdMob for now - it's broken in 7.69.0-cppsdk.

* Don't process swift headers if already processed

* fix nightly version tag

* Fix update dependencies script

* Support directories with spaces

* Exclude swift headers from linter.

* Fix whitespace

* Fix comma

* Fix whitespace.

* Fix Firestore external file.

* Restore new external file and fix include path.

* Fix include files and paths to be compatible with C++ build.

* Format code.

* Add NOLINT

* Remove included test with bad include path.

* Fix internal test podfile

* Fix podfiles

* Revert some junk that got pulled in when #898 was merged into this branch

* Temporarily remove admob from integration_tests.yml too.

* integration_tests.yml: remove admob from another place, since it still seems to be being built

* Revert "integration_tests.yml: remove admob from another place, since it still seems to be being built"

This reverts commit fc6b845.

* Revert "Temporarily remove admob from integration_tests.yml too."

This reverts commit 32aec7e.

* Add AdMob back in and refer to new version in staging.

* Speed up update-dependencies zip distribution usage

* Fix substitution.

* Fix header formatting

* Fix spacing.

* Update Xcode version to 12.5.1 and add tests to 13.2.1

* Add new version to readme

* Change Functions iOS SDK and tvOS SDK version to 12.4 (minimum for Swift).

* Since FIRStorageMetadata no longer implements NSCopying, copy it a different way.

* Change deployment version to 12.4 for Storage, required for Swift support.

* Update analytics headers from 9.0.0 Analytics RC.

* Add note about removal of deprecated analytics constants.

* Set analytics deployment target

* Set deployment targets

* Add empty Swift file to sample framework.

* Switch Podfile postprocessing to use python3

* Add empty Swift file to integration test project.

* Add empty Swift file to analytics project

* Add empty.swift to remaining integration test projects.

* Check to make sure that the controller can pause/resume/etc.

* Workaround for pause/cancel/resume semantics changing in 9.x.

* Switch from nightly to release tag for Firestore external dep.

* Update Xcode version to 13.2.1, required version for iOS SDK.

* Change deployment target back to 10.x.

* Fix iOS deployment version back to 10.0

* Add notes about including a swift file

* Change comparison to lower-case to prevent restore_secrets overriding it.

* Clean up the verbose log of file size

* firestore_snappy.patch.txt: remove the patching for firebase/firebase-ios-sdk#9662 (Create individual Python virtualenv's in cmake builds) since it's incorporated into the CocoaPods-9.0.0.nightly tag

* CHERRY PICK from main branch: leveldb_snappy_test.cc: run the test on iOS as well, and other improvements (#896)

This change improves the leveldb_snappy_test.cc to provide stronger guarantees of expected behavior.

* Set block storage for task variable.

* Fix server key.

* Format code.

* test iOS simulator devices

* Windows compatibility for strcasecmp

* test iOS simulator devices

* Update Swift header to include copy method; revert Storage to use it.

* upload ios projects artifacts

* Temporarily disable a test that fails on device when built from cmdline.

* Update the testing Xcode version to 13.3.1

* Change xcode version to 13.3.1 and macos runners to macos-12.

* Restore simulator versions back to previous values

* Reworded some release notes.

(Also replaced a couple tabs with spaces.)

* Revert "upload ios projects artifacts"

This reverts commit e04d6a6.

* Fix Swift header script to work with multiple URLs.

* Update Swift headers for final 9.0.0 release

* Add readme about ios_pod directory and its swift_headers subdirectory

* Remove staging repo from podfiles. (#923)

* If the zip file can't be downloaded, just skip that step.

Co-authored-by: Denver Coneybeare <[email protected]>
Co-authored-by: Mou <[email protected]>
Co-authored-by: Mou Sun <[email protected]>
Co-authored-by: a-maurice <[email protected]>
@firebase firebase locked and limited conversation to collaborators May 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api: firestore skip-release-notes Skip release notes check tests: succeeded This PR's integration tests succeeded.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants