[bisq-network/bisq] (7/8) Develop APIs for ProtectedStorageEntry to enable testing and code refactoring (#3583)

Julian Knutsen notifications at github.com
Sat Nov 9 03:10:29 UTC 2019 

New commits start e33498d

I expect a bit of discussion on this one based on my previous feedback, so I wanted to outline my thoughts here so that the best path forward can be taken with regards to testing, maintainability, and clean programming practices.

This entire stack was based on testing the P2PDataStore and refactoring it so it is more maintainable and easier to add new features in the future.

The module was almost impossible to test when starting out. I decided to write very heavy-handed integration tests while looking at the implementation to verify I covered all the branches and edge cases. The end result #3554 allowed refactoring to be done while ensuring the various combinations of ProtectedStorageEntrys and Payloads still worked correctly.

But in order to do that, the tests needed to create REAL ProtectedStorageEntry & ProtectedMailboxStorageEntry objects from scratch and manipulate them into the various valid and invalid configurations to exercise the code. Real keys, real hashes, a lot of overhead for testing that should be pretty straight forward.

The issue is that too much of the code exists in private helper functions inside the P2PDataStore itself. The only testing that can be done is front door integration testing where the tests know way too much about the implementation.

My solution to the complexity that I present in the next 2 pull requests is to turn ProtectedStorageEntry into a real object with a real API that can be unit tested. Determining whether or not an Entry is valid for an add operation shouldn't depend on the P2PDataStore internal state, or have to be run through the Client API to verify correctness.

If you are at all interested, I urge you to look at the unit tests in the following pull request (ProtectedStorageEntryTest & ProtectedMailboxStorageEntryTest) and see how easy it is to verify the correctness of an add or remove operation versus the path the integration test had to take in the beginning.

In addition to adding the ability to unit test the Entrys, real unit tests can now exist for the P2PDataStore. Instead of constructing a real object, they now just build a mock Entry (something that wasn't possible with the old object relationship). The result is that the unit tests for P2PDatastore can test only the code they need to (what data structures to update, who to signal, etc) and not worry about the implementation specifics of Mailbox vs. Normal Entrys.

The end result is code that has unit tests, has integration tests, is easy to navigate, and has line and branch coverage over 80%. More important than the numbers is that a developer can go in and change the code and get feedback on whether or not it worked. Something that wasn't possible before.

In summary, I would request that anyone who is interested look through the testing and weigh in on the maintainability and testing strategy of this refactor. The reality is that this code wasn't tested, has plenty of bugs, and was hard to maintain and develop against. It is only one of many that likely have the same problem and starting to have a discussion on the practices that everyone wants in Bisq should happen sooner than later.

Pull Request (7/8)

This pull request will introduce the API, test it, and use the new benefits to combine the remove() and removeMailboxData() paths. 

Pull Request(8/8)

This pull request will make use of the new objects to create clean mocks that clean up the integration tests and remove a lot of the heavy-handed object creation.

If you got this far, thanks again for taking the time to look.

You can view, comment on, or merge this pull request online at:

  https://github.com/bisq-network/bisq/pull/3583

-- Commit Summary --

  * [TESTS] Add tests for P2PDataStorage in order to safely refactor
  * [BUGFIX] Call correct remove function in addMailboxData failure path.
  * [REFACTOR] P2PDataStorage::addPersistableNetworkPayload()
  * [REFACTOR] P2PDataStorage::addProtectedStorageEntry()
  * [REFACTOR] P2PDataStorage::refreshTTL()
  * [REFACTOR] P2PDataStorage::remove()
  * [REFACTOR] P2PDataStorage::removeMailboxData()
  * Update behavior of P2PDataStorage::addProtectedStorageEntry() on duplicates
  * Update behavior of P2PDataStorage::refreshTTL() on duplicates
  * Update behavior of P2PDataStorage::remove() & removeMailboxData() on duplicate sequence #s
  * [PR COMMENTS] Don't use the real Alert class
  * Merge branch 'add-tests' into fix-remove-bug
  * [PR COMMENTS] Don't call removeMailboxData in addMailboxData failure path
  * Merge branch 'fix-remove-bug' into refactor-add-remove-refresh
  * [PR COMMENTS] Clean up logging messages
  * Merge branch 'refactor-add-remove-refresh' into update-duplicate-behavior
  * [TESTS] Reduce concurrent failure tests to 2 concurrent operations instead of 3
  * Merge branch 'master' into add-tests
  * [PR COMMENTS] Use Client API in comments
  * Merge branch 'add-tests' into fix-remove-bug
  * [PR COMMENTS] Remove concurrent tests
  * Merge branch 'fix-remove-bug' into refactor-add-remove-refresh
  * [PR COMMENTS] Use "Client API" in transient comments
  * Merge branch 'refactor-add-remove-refresh' into update-duplicate-behavior
  * Use dependency injected Clock in P2PDataStore
  * [REFACTOR] Clean up ProtectedStorageEntry ctor
  * Add Clock parameter to ProtectedStorageEntry
  * Use Clock instead of System in ProtectedStorageEntry
  * [TESTS] Use ClockFake in tests to control time
  * [TESTS] Test onBootstrapComplete()
  * [TESTS] Lower entries required for purge in tests
  * [PR COMMENTS] Don't store Clock in ProtectedStorageEntry
  * Merge branch 'master' into add-tests
  * Merge branch 'add-tests' into fix-remove-bug
  * Merge branch 'fix-remove-bug' into refactor-add-remove-refresh
  * Merge branch 'refactor-add-remove-refresh' into update-duplicate-behavior
  * Merge branch 'update-duplicate-behavior' into manual-clock-in-tests
  * [REFACTOR] Remove duplicated code in refreshTTL
  * [TESTS] Add more tests around mis-wrapped ProtectedStorageEntrys
  * [REFACTOR] ProtectedStorageEntry::validForAddOperation()
  * [REFACTOR] ProtectedStorageEntry::isValidForRemoveOperation()
  * [REFACTOR] Remove checkPublicKeys()
  * Clean up toString() methods
  * [REFACTOR] Move signature validation behind isValidForAddOperation()
  * [REFACTOR] Move signature validation behind isValidForRemoveOperation()
  * [REFACTOR] Move receiversPubKey check behind isValidForRemoveOperation()
  * Remove duplicate check in refreshTTL
  * Introduce isMetadataEquals and use it
  * [TESTS] Update remove validation with BroadcastMessage type
  * Combine remove() and removeMailboxData()

-- File Changes --

    M common/src/main/java/bisq/common/proto/network/NetworkProtoResolver.java (4)
    M core/src/main/java/bisq/core/proto/CoreProtoResolver.java (6)
    M core/src/main/java/bisq/core/proto/network/CoreNetworkProtoResolver.java (5)
    M monitor/src/main/java/bisq/monitor/metric/P2PNetworkLoad.java (6)
    M monitor/src/main/java/bisq/monitor/metric/P2PSeedNodeSnapshotBase.java (4)
    M p2p/src/main/java/bisq/network/p2p/P2PService.java (12)
    M p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java (442)
    M p2p/src/main/java/bisq/network/p2p/storage/payload/ProtectedMailboxStorageEntry.java (161)
    M p2p/src/main/java/bisq/network/p2p/storage/payload/ProtectedStorageEntry.java (183)
    M p2p/src/test/java/bisq/network/p2p/TestUtils.java (5)
    M p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageTest.java (1732)
    M p2p/src/test/java/bisq/network/p2p/storage/ProtectedDataStorageTest.java (4)
    M p2p/src/test/java/bisq/network/p2p/storage/messages/AddDataMessageTest.java (4)
    A p2p/src/test/java/bisq/network/p2p/storage/mocks/AppendOnlyDataStoreServiceFake.java (42)
    A p2p/src/test/java/bisq/network/p2p/storage/mocks/ClockFake.java (49)
    A p2p/src/test/java/bisq/network/p2p/storage/mocks/DateTolerantPayloadStub.java (50)
    A p2p/src/test/java/bisq/network/p2p/storage/mocks/PersistableNetworkPayloadStub.java (43)
    A p2p/src/test/java/bisq/network/p2p/storage/mocks/ProtectedDataStoreServiceFake.java (45)
    A p2p/src/test/java/bisq/network/p2p/storage/mocks/ProtectedStoragePayloadStub.java (64)
    A p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedMailboxStorageEntryTest.java (202)
    A p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedStorageEntryTest.java (216)
    A p2p/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker (1)

-- Patch Links --

https://github.com/bisq-network/bisq/pull/3583.patch
https://github.com/bisq-network/bisq/pull/3583.diff

-- 
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/3583
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20191108/31723c27/attachment.html>

More information about the bisq-github mailing list