(1/8) [TESTS] Add tests for P2PDataStorage in order to safely refactor #3554
Conversation
These tests create real versions of the supported Payload & Entry types and run them through the 3 entry points (onMessage, Init, standard add()/remove()/refresh(), to verify the expected return values, internal state changes, and external signals (listeners, broadcasts). The tests are involved and I am proposing future work to make many of the objects more testable that will greatly reduce the work and tests cases needed. This work identified a few unexpected scenarios and potential bugs that are addressed in dependent pull requests. Code coverage when running P2PDataStorageTest: Before: Line: 4% Branch: 0% After: Line: 78% Branch 76%
julianknutsen
changed the title
build.gradle
Outdated
| @@ -221,6 +221,7 @@ configure(project(':p2p')) { | |||
| testCompileOnly "org.projectlombok:lombok:$lombokVersion" | |||
| testAnnotationProcessor "org.projectlombok:lombok:$lombokVersion" | |||
| testCompile("org.mockito:mockito-core:$mockitoVersion") | |||
| testCompile project(':core') | |||
There was a problem hiding this comment.
The p2p module has not dependency on core (Core has on p2p). I think it is better to not break that in the tests as well.
As far I see that dependency is only for the Alert class needed. I would recommend to create a custom Mockclass instead of extending Alert.
There was a problem hiding this comment.
Agreed. This was done early on when I didn't understand the protobuf hashing interaction. The new stub class makes the tests much nicer.
| @@ -47,7 +47,7 @@ | |||
| @Getter | |||
| @ToString | |||
| @Slf4j | |||
| public final class Alert implements ProtectedStoragePayload, ExpirablePayload { | |||
| public class Alert implements ProtectedStoragePayload, ExpirablePayload { | |||
There was a problem hiding this comment.
We try to keep final for all objects which are sent over the wire. Before we used protobuf we used Java serialisation and it was a security risk to not make it final (an arbitrary extended class would have passed some instance checks). With protobuf I am not aware of a security risk in that regards, but I think its better to be paranoid in that regards und keep the classed final as far as possible. In that case it also introduces that dependency to core so I thing we should just use a custom mock instead and both problems are solved.
|
@freimair Could you review as well? I did not review much the tests... |
Instead, create a ProtectedStoragePayloadStub class which mocks out the required protobuf Message for hashing. The hash is equal to the ownerPubKey so they are unique.
Change the use of "public api" to "Client API" to describe the set of callers that use the pattern addProtectedStorageEntry(getProtectedStorageEntry()) as a contrast to the onMessage handler users or the GetData users.
julianknutsen
changed the title
p2p/src/test/java/bisq/network/p2p/storage/mocks/AppendOnlyDataStoreServiceFake.java
Show resolved
Hide resolved
p2p/src/test/java/bisq/network/p2p/storage/mocks/ProtectedDataStoreServiceFake.java
Show resolved
Hide resolved
|
As far as splitting up the tests, you are going to run into a situation where helper files are needed for the common test code or the parent unit test fixture and navigating the tests to understand what is going on is even harder to read (I went down this path initially). The reality is that this test code is complicated and has a ton of embedded/shared code because the module was extremely hard to test in the current state. The goal of this first patch was to get everything fully tested in the "best" way possible, so the future work could refactor it into something much easier to work with. The version of the tests after (#3584) are much cleaner after the unit tests of ProtectedStorageEntry and ProtectedMailboxStorageEntry could be added. If there is still too much going on in one file after #3584, I can try to take a look at splitting them up there, but doing so at 1/8 will make it impossible to merge any of the remaining code that all leverages this test code. |
|
I've taken your feedback on splitting up the tests and the result is in #3587. There are now 6 separate test files that test separate entry points or functionality and a few additional helper classes that are shared between tests. I've also added plenty of JavaDocs to help future developers understand how the tests work and what they are testing. As I mentioned in the (9/9) PR, I would prefer to leave this split to the end of my request stack due to the number of merges that would be required in the interim pull requests. If the maintainer isn't interested in the refactor I can possibly merge the split back to this point so you guys can have the tests moving forward. Thanks for reviewing my work and giving feedback. |
|
@julianknutsen @freimair |
|
All of these incremental ones can be individually reviewed and merge. They are all just branches of the previous PR in the stack that I have been merging down to keep them consistent with master and each other. After doing this style for a week, it is really busy and hard to read after 1-2 resyncs from master and 1-2 change requests. I want to make this as easy as possible on you guys and understand the best way forward for future patches. I can either:
I like the idea of having small incremental PRs so that if there is some design discussion and rework in 7/8, the previous patches that include bugfixes and tests can still make it in and raise the quality. But, I think choosing something that works best for you guys as the reviewers to make it easy to audit and verify changes is the best path forward. Thanks! |
These tests create real versions of the supported Payload & Entry types and
run them through the 3 entry points (onMessage, Init, standard add()/remove()/refresh(),
to verify the expected return values, internal state changes, and
external signals (listeners, broadcasts).
The tests are involved and I am proposing future work to make many of the objects
more testable that will greatly reduce the work and tests cases needed.
This work identified a few unexpected scenarios and potential bugs that are addressed
in dependent pull requests.
Code coverage when running P2PDataStorageTest:
Before:
Line: 4%
Branch: 0%
After:
Line: 78%
Branch 76%