[bisq-network/bisq] (1/4) [TESTS] Add tests for P2PDataStorage in order to safely refactor (#3554)

[chimp1984]

chimp1984 requested changes on this pull request.



> @@ -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')

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.

> @@ -47,7 +47,7 @@
 @Getter
 @ToString
 @Slf4j
-public final class Alert implements ProtectedStoragePayload, ExpirablePayload {
+public class Alert implements ProtectedStoragePayload, ExpirablePayload {

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.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/bisq-network/bisq/pull/3554#pullrequestreview-311460743
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20191104/38e7b63a/attachment.html>