-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Add tests for ppl 1 #14885
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
base: cheryllin/ppl
Are you sure you want to change the base?
Add tests for ppl 1 #14885
Conversation
self = [super init]; | ||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @wu-hui , do you have any opinions for this?
@@ -195,8 +195,8 @@ NS_SWIFT_NAME(UnnestStageBridge) | |||
@end | |||
|
|||
NS_SWIFT_SENDABLE | |||
NS_SWIFT_NAME(GenericStageBridge) | |||
@interface FIRGenericStageBridge : FIRStageBridge | |||
NS_SWIFT_NAME(AddStageBridge) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to confirm, this is what we decided on?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Andy, are you suggesting test using servertimestamp instead of local machine time?
|
||
print(snapshot) | ||
} | ||
|
||
func testEmptyResults() async throws { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
#no-changelog