-
Notifications
You must be signed in to change notification settings - Fork 134
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
RUMM-437 Encoding doesn't ignore miliseconds anymore #96
Conversation
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.
let date = {2020-05-04T18:19:57.109Z} jsonEncoder.dateEncodingStrategy = .iso8601 jsonEncoder.encode(date) // becomes {2020-05-04T18:19:57Z}
That's super interesting 👌. Is this factorial precision lost somewhere documented by Apple?
How can we test encoding without making types Decodable, since they don't need to be decoded in the main target?
We should unit test the iso8601Formatter
by its own and just assert that the encoded String
includes factorial seconds:
func testEncodingDateWithISO8601FullFormat() throws {
let jsonEncoder = JSONEncoder()
jsonEncoder.dateEncodingStrategy = .formatted(iso8601Formatter)
let knownDate: Date = .mockDecember15th2019At10AMUTC(addingTimeInterval: 0.123)
let encodedKnownDate = try jsonEncoder.encode(knownDate)
let jsonDecoder = JSONDecoder()
let knownDateDecodedString = try jsonDecoder.decode(String.self, from: encodedKnownDate)
XCTAssertEqual(knownDateDecodedString, "2019-12-15T10:00:00.123")
}
Higher-level tests should just work, because:
- we only have a few precisely picked unit tests which assert the date format - with
LoggerTests.testSendingMinimalLogWithDefaultLogger()
being the most crucial one; - they work against
.mockDecember15th2019At10AMUTC()
mock, which doesn't include factorial seconds.
f67b725
to
3026b9a
Compare
@ncreated I was thinking of a more complete encoding testing strategy (not only |
3026b9a
to
f9a6884
Compare
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.
✨ 🚀
Closes #95
What and why?
SDK collects
Log
s and writes them onto disk, then uploads them as raw dataIt relies on
JSONEncoder
withdateEncodingStrategy = .iso8601
However, it turns out
IS08601
doesn't enforce miliseconds andJSONEncoder
omits themThe result is very well described in the original issue #95
How?
There are 2 main solutions:
ISO8601DateFormatter
anddateEncodingStrategy = .custom
🍎 Apple recommends using
ISO8601DateFormatter
when dealing withISO8601
format.❌ However, that enforces us to use
dateEncodingStrategy = .custom
Because the logical strategy option,
.formatted(DateFormatter)
, doesn't accept it as it's not a subclass ofDateFormatter
Customized
DateFormatter
anddateEncodingStrategy = .formatted
Although this is not the official way, this method is used in many places/projects.
Basically, we describe the
ISO8601
standard in our ownDateFormatter
instanceThen we pass the instance to
dateEncodingStrategy = .formatted(dateFormatter)
✅ Now we have miliseconds in the encoded data
UPDATE:
I turned to the first option:
ISO8601DateFormatter
anddateEncodingStrategy = .custom
Questions?
How can we test encoding without making types
Decodable
, since they don't need to be decoded in the main target?Review checklist