firebase-ios-sdk icon indicating copy to clipboard operation
firebase-ios-sdk copied to clipboard

Verify that @DocumentID of String works with custom Codable methods.

Open peterfriese opened this issue 3 years ago • 3 comments

In one of my apps, I had to deal with a situation that called for implementing custom encoding / decoding. This PR adds tests to verify custom Codable methods work with @DocumentID if the underlying attribute is of type String.

This is related to #7247

#no-changelog

peterfriese avatar Jan 27 '21 12:01 peterfriese

1 Warning
:warning: Did you forget to add a changelog entry? (Add #no-changelog to the PR description to silence this warning.)

Generated by :no_entry_sign: Danger

google-oss-bot avatar Jan 27 '21 12:01 google-oss-bot

This seems unnecessary.

We already have unit tests that test various permutations of @DocumentID: https://github.com/firebase/firebase-ios-sdk/blob/88ecc3cd5fc55291eca07ccc9a0209fb8c222f0b/Firestore/Swift/Tests/Codable/FirestoreEncoderTests.swift#L574

While these don't specifically cover custom Codable models, these do verify that @DocumentID does the right thing when wrapping a String.

Integration tests are far more expensive in terms of run time than unit tests, so the integration test for @DocumentID only directly covers DocumentID<DocumentReference>. It shows that @DocumentID works at all when encountering real document references, without verifying all the different ways it can be used.

The testSelfDocumentIDWithCustomCodable test I added in #7247 originally started as just trying to figure out how to use DocumentID in custom Codable implementations. While this test technically does not add any new coverage, this was tricky enough to make work at all that I wanted to leave this in the suite, essentially as an example that shows this is possible. However, once we've delegated to DocumentID, the existing unit tests cover wrapped type permutations again.

These new tests don't really add any new coverage and do extend suite run time so I'm disinclined toward merging this.

wilhuff avatar Jan 27 '21 18:01 wilhuff

@peterfriese Should we close this one?

paulb777 avatar Oct 08 '22 00:10 paulb777