(5/5) [TESTS] Clean up mockito never() and eq(null) usages #3671
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This will allow us to push the GetData creation inside P2PDataStorage safely.
As part of changing the GetData path, we want to move all creation and processing of GetData messages inside P2PDataStorage. This will allow easier unit testing of the behavior as well as cleaner code in the request handlers that can just focus on nonces, connections, etc.
These are identical test cases to the requestHandler tests, but with much fewer dependencies. The requestHandler tests will eventually be deleted, but they are going to remain throughout development as an extra safety net.
Add some basic sanity tests prior to the refactor to help catch issues.
This is just a strict move of code to reduce errors.
Changed the log to reference getDataResponse instead of getData. Now that we might truncate the response, it ins't true that this is exactly what the peer asked.
Move the logging that utilizes connection information into the request handler. Now, buildGetDataResponse just returns whether or not the list is truncated which will make it easier to test.
Remove the dependence on the connection object by having the handler pass in the peer's capabilities. This now allows unit testing of buildGetDataResponse without any connection dependencies.
Write a full set of unit tests for buildGetDataResponse. This provides a safety net with additional refactoring work.
The appendOnlyDataStoreService and map already have unique keys that
are based on the hash of the payload. This would catch instances
where:
PersistableNetworkPayload
- None: The key is based on ByteArray(payload.getHash()) which is the
same as this check.
ProtectedStorageEntry
- Cases where multiple PSEs contain payloads that have equivalent
hashCode(), but different data.toProtoMessage().toByteArray().
I don't think it is a good idea to keep 2 "unique" methods on
payloads. This is likely left over from a time when
Payload hashCode() needed to be different than the hash of
the payload.
Move the logging function to the common capabilities check so it can run on both ProtectedStoragePayload and PersistableNetworkPayload objects
Move the capability check inside the stream operation. This should improve performance slightly, but more importantly it makes the two filter functions almost identical so they can be combined.
Removes unnecessary calculations converting Set<byte[]> into Set<ByteArray> and allows additional deduplication of stream operations.
Introduce a generic function that can be used to filter Map<ByteArray, PersistableNetworkPayload> or Map<ByteArray, ProtectedStorageEntry>. Used to deduplicate the GetData code paths and ensure the logic is the same between the two payload types.
Now, the truncation is only triggered if more than MAX_ENTRIES could have been returned.
Add heavy-handed test that exercises the logic to use as a safeguard for refactoring.
Just a code move for now.
Now that we want to unit test the GetData path which has different behavior w.r.t. broadcasts, the tests need a way to verify that state was updated, but not broadcast during an add. This patch changes all verification function to take each state update explicitly so the tests can do the proper verification.
Add a full set of unit tests that uncovered some unexpected behavior w.r.t. signalers.
Previously, multiple handlers needed to signal off one global variable. Now, that this check is inside the singleton P2PDataStorage, make it non-static and private.
Write a few integration test that exercises the exercise interesting synchronization states including the lost remove bug. This fails with the proper validation, but will pass at the end of the new feature development.
- Add more comments - Use Clock instead of System - Remove unnecessary AtomicInteger
Name is left over from previous implementation. Change it to be more relevant to the current code and update comments to indicate the current usage.
Checking for null creates hard-to-read code and it is simpler to just create an empty set if we receive a pre-v0.6 GetDataResponse protobuf message that does not have the field set.
The only two users of this constructor are the fromProto path which already creates an empty Capabilities object if one is not provided and the internal usage of Capabilities.app which is initialized to empty. Remove the @nullable so future readers aren't confused.
…quest The only two users of this constructor are the fromProto path which now creates an empty Capabilities object similar to GetDataResponse. The other internal usage of Capabilities.app which is initialized to empty.
Now that all the implementations are unit tested in P2PDataStorage, the old tests can be removed.
Now that the only user is internal, the API can be made private and the tests can be removed. This involved adding a few test cases to processGetDataResponse to ensure the invalid hash size condition was still covered.
Now that more callers have moved internal, the public facing API can be cleaner and more simple. This should lead to a more maintainable API and less sharp edges with future development work.
Remove isDataOwner from the client API. All users pass in true. All test users don't care.
Remove isDataOwner from the client API. All users pass in true. All test users don't care.
Remove isDataOwner from the client API. All users pass in true. All test users don't care.
isDataOwner is used when deciding how many peer nodes should receive a BroadcastMessage. If the BroadcastMessage originated on the local node it is sent to ALL peer nodes with a small delay. If the node is only relaying the message (it originated on a different node) it is sent to MAX(peers.size(), 7) peers with a delay that is twice as long. All the information needed to determine whether or not the BroadcastMessage originated on the local node is available at the final broadcast site and there is no reason to have callers pass it in. In the event that the sender address is not known during broadcast (which is only a remote possibility due to how early the local node address is set during startup) we can default to relay mode. This first patch just removes the deep parameters. The next will remove everything else. There is one real change in LiteNodeNetworkService.java where it was using the local node when it should have been using the peer node. This was updated to the correct behavior.
Remove all usages in code and tests now that the behavior is internal to the BroadcastHandler
Now that it is identical the the broadcaster version it can be inlined.
Now that it is identical the the broadcaster version it can be inlined.
Now that there is only one user it can be inlined.
never() and any() don't play well together for nullable types. Change the broadcast mocks to user nullable() and fixup tests.
julianknutsen
force-pushed
the
robust-test-asserts
branch
from
fd38c61
to
17f4b70
Compare
|
Rebased against master and published now that v1.2.4 is branched |
julianknutsen
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
17f4b70
Mockito's never() and any() don't play well together for nullable types. Change the broadcast mocks to user nullable() and fixup tests appropriately.