Conversation
cherylEnkidu
added
api: firestore
and removed
api: messaging
api: storage
api: firestore
api: dynamiclinks
api: performance
api: remoteconfig
api: functions
api: inappmessaging
| if (self) { | ||
| collection_source = std::make_shared<CollectionSource>(MakeString(path)); | ||
| if (ref.firestore.databaseID.CompareTo(db.databaseID) != ComparisonResult::Same) { | ||
| ThrowInvalidArgument( |
There was a problem hiding this comment.
I am leaning towards propagating this error to the Swift layer and making it a fatal error to avoid introducing more throw statement in objective-c layer.
The reason for not making it throwable is I would like not to make the db.pipeline().collection("") a throwable function.
For more info: go/firestore-ios-sdk-errors
There was a problem hiding this comment.
Note: There would be some effort to test the fatal error in Swift, since there is no build in functions in Swift library for that.
There was a problem hiding this comment.
Hi @wu-hui , do you have any opinions for this?
There was a problem hiding this comment.
Agree with your assessment.
| NS_SWIFT_SENDABLE | ||
| NS_SWIFT_NAME(GenericStageBridge) | ||
| @interface FIRGenericStageBridge : FIRStageBridge | ||
| NS_SWIFT_NAME(AddStageBridge) |
There was a problem hiding this comment.
Just to confirm, this is what we decided on?
There was a problem hiding this comment.
Sorry, the latest decision is go with rawStage I will update this PR.
|
|
||
| let executionTimeValue = snapshot.executionTime.dateValue().timeIntervalSince1970 | ||
|
|
||
| XCTAssertGreaterThanOrEqual( |
There was a problem hiding this comment.
Depending on the environment this test runs, it can fail if the machine's time is wonky.
You can execute the pipeline twice and make sure the timestamp moves forward instead.
There was a problem hiding this comment.
Hi Andy, are you suggesting test using servertimestamp instead of local machine time?
There was a problem hiding this comment.
I see what you mean, I will update the test
There was a problem hiding this comment.
Sincer the tests description only check for whether the result has the time data, so I remove all local time statements
| print(snapshot) | ||
| } | ||
|
|
||
| func testEmptyResults() async throws { |
There was a problem hiding this comment.
Is this the full set of tests? I feel like there is more to port from web, can you check?
There was a problem hiding this comment.
No, it is not the full tests set.
I port the test in sequence, and separate them into several PRs. The reason for that is it also have bug fixes need to be merged ASAP to feature branch, and I don't want to make a large PR.
4 participants
#no-changelog