Skip to content

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

Open
wants to merge 54 commits into
base: cheryllin/ppl
Choose a base branch
from
Open

Add tests for ppl 1 #14885

wants to merge 54 commits into from

Conversation

cherylEnkidu
Copy link
Contributor

#no-changelog

self = [super init];
if (self) {
collection_source = std::make_shared<CollectionSource>(MakeString(path));
if (ref.firestore.databaseID.CompareTo(db.databaseID) != ComparisonResult::Same) {
ThrowInvalidArgument(
Copy link
Contributor Author

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

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)
Copy link
Contributor

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?

Copy link
Contributor Author

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(
Copy link
Contributor

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.

Copy link
Contributor Author

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 {
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@wu-hui wu-hui assigned cherylEnkidu and unassigned wu-hui Jun 6, 2025
@cherylEnkidu cherylEnkidu assigned wu-hui and unassigned cherylEnkidu Jun 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants