Conform AddressedEnvelope conditionally to Hashable & Equatable
#2017
Conversation
FranzBusch
force-pushed
the
feature/addressed-envelope-hashable
branch
from
1a6d2b3 to
35210be
Compare
FranzBusch
force-pushed
the
feature/addressed-envelope-hashable
branch
from
35210be to
f945864
Compare
| let envelope2 = AddressedEnvelope(remoteAddress: address, data: "bar") | ||
|
|
||
| XCTAssertNotEqual(envelope1, envelope2) | ||
| XCTAssertNotEqual(envelope1.hashValue, envelope2.hashValue) |
There was a problem hiding this comment.
Two different values can actually produce the same hash value. It's unlikely but as hashes are different on each execution this could be a flaky test.
There was a problem hiding this comment.
hm... as we just use the automatically synthesised conformance of Hashable I would argue that it is not really necessary to actually test this specific case. @glbrntt WDYT?
There was a problem hiding this comment.
True, but the likely hood of these actually colliding with the given input is slim to none. Sadly, I think there is no option to set SWIFT_DETERMINISTIC_HASHING for our target as an environment variable.
There was a problem hiding this comment.
If you really want to test it you can do it like the standard library. They are hashing the values up to 10 times with different "seeds". At least one hash value needs to be different:
https://github.com/apple/swift/blob/8e154a415a2d0467690db69bac1d6090fc8b4852/stdlib/private/StdlibUnittest/StdlibUnittest.swift#L3025-L3036
To change the "seed" at runtime they just hash another integer into the hasher:
https://github.com/apple/swift/blob/8e154a415a2d0467690db69bac1d6090fc8b4852/stdlib/private/StdlibUnittest/StdlibUnittest.swift#L2892-L2903
There was a problem hiding this comment.
That's super interesting! Thanks for the links. But I would agree with you before we go to that length, we can also just trust that the default Hashable synthesised code is working properly.
There was a problem hiding this comment.
hm... as we just use the automatically synthesised conformance of Hashable I would argue that it is not really necessary to actually test this specific case. @glbrntt WDYT?
That's a really good point. I don't think it's necessary but I'm also not opposed to having a test for it, it will be -- at the very least -- and extra line of defence against accidentally removing it (although I'd really hope that the API checker and reviewers would notice that!).
There was a problem hiding this comment.
Alternatively, as we don't care about the actual hash value, shoving them in a Set and checking the size is as expected should also be fine.
There was a problem hiding this comment.
I was thinking about the set "hack" as well. If that is fine for you all, I would implement that.
There was a problem hiding this comment.
shoving them in a Set and checking the size is as expected should also be fine.
I don't think this adds any value either. Set will happily accept multiple values with the same hashValue as long as they are not equal.
We should already be fine with the first test where we test that two equal values also produce the same hashValue. In this test we access the hashValue property and therefore make sure the type is indeed Hashable.
BTW: hashValue is actually deprecated but I'm fine with using it in tests
There was a problem hiding this comment.
Okay, let's do the following. I will only keep the hashValue test for the equal case and drop it everywhere else 😀
I share your concerns w.r.t. hashValue and its deprecation, but I think for testing it is fine to use.
### Motivation: Sometimes, it is nice to check `AddressedEnvelope`s for equality or hash them. Especially, in tests this comes in handy. ### Modifications: This PR, adds conditional conformances to `AddressedEnvelope` for `Equatable` and `Hashable` depending on its generic `DataType` . I also added a new module `NIOCoreTests` which was an open todoleft from the creation of the `NIOCore` module. To not add more tests that we have to migrate in the future, I started to create the new tests in the new module right away. I also created issue apple#2016, to keep track of our open task to move the other tests over. ### Result: `AddressedEnvelope` is conditionally `Equatable` and `Hashable`
FranzBusch
force-pushed
the
feature/addressed-envelope-hashable
branch
from
f945864 to
f98eef7
Compare
3 participants
Motivation:
Sometimes, it is nice to check
AddressedEnvelopes for equality or hash them. Especially, in tests this comes in handy.Modifications:
This PR, adds conditional conformances to
AddressedEnvelopeforEquatableandHashabledepending on its genericDataType.I also added a new module
NIOCoreTestswhich was an open todoleft from the creation of theNIOCoremodule. To not add more tests that we have to migrate in the future, I started to create the new tests in the new module right away. I also created issue #2016, to keep track of our open task to move the other tests over.Result:
AddressedEnvelopeis conditionallyEquatableandHashable