pss: Refactor. Step 2. Refactor forward cache#1742
pss: Refactor. Step 2. Refactor forward cache#1742nolash merged 8 commits intoethersphere:masterfrom
Conversation
c41b301 to
9b62bd3
Compare
9b62bd3 to
5205a71
Compare
| @@ -0,0 +1,54 @@ | |||
| package ticker | |||
There was a problem hiding this comment.
I am guessing that the reason for including a new dependency here is quick time travel to replace the original sleeps.
In this case it is being used for a rather simple test. Do you really feel it's warranted to invite external code in just for this?
There was a problem hiding this comment.
I would like to introduce the practice of injecting the clock as a dependency for each component that requires a time reference throughout Swarm. I missed this in Feeds.
This tilina/clock package takes care of offering both a mock clock for tests and a passthrough one for runtime, both implementing the same interface, which is identical to time, so people know what to expect. The lib is small and well tested.
Code that uses time.Now() directly cannot be tested reliably. A component should use the injected time reference.
As a result, all these tests run predictably and instantaneously.
If this is approved, I'd like to refactor Feeds to use it as well.
There was a problem hiding this comment.
I understand the advantage of it. It's fine by me, and I trust you when you vouch for it.
But this might end up being a discussion of course, so when you say:
well tested
maybe for the record say something about what that means? Well tested by you? Other endorsements? Even though it's small we should still be mindful.
To be honest I don't know the protocol for adding new deps to Swarm. I am sure for example @janos does.
There was a problem hiding this comment.
Well tested means I read the tests and are sufficient. I would have written it myself following the same pattern and include it in Swarm if necessary.
The last thing I want is to start a discussion. If this is approach is not accepted I will remove it.
There was a problem hiding this comment.
This PR has a new dependency added nicely. There is no problem with that. And for the clock dependency itself, I think that it has very nice api and that it is useful. Go lack of mocking time is very annoying. We can argue if another similar package can be used, given the popularity and development activity for this one compared to others. But I think that it is ok.
| @@ -0,0 +1,54 @@ | |||
| package ticker | |||
There was a problem hiding this comment.
I understand the advantage of it. It's fine by me, and I trust you when you vouch for it.
But this might end up being a discussion of course, so when you say:
well tested
maybe for the record say something about what that means? Well tested by you? Other endorsements? Even though it's small we should still be mindful.
To be honest I don't know the protocol for adding new deps to Swarm. I am sure for example @janos does.
* 'master' of github.com:ethersphere/swarm: (32 commits) network/stream: refactor cursors tests (ethersphere#1786) network: Add capabilities if peer from store does not have it (ethersphere#1791) Swap logger (ethersphere#1754) network: Add capability filtered depth calculation (ethersphere#1787) travis: remove go1.12 job (ethersphere#1784) cmd/swarm: correct bzznetworkid flag description (ethersphere#1761) network, pss: Capability in pss (ethersphere#1764) network/stream: handle nil peer in TestNodesExchangeCorrectBinIndexes (ethersphere#1779) protocols, retrieval: swap-enabled messages implement Price (ethersphere#1771) cmd/swarm-smoke: fix waitToPushSynced connection closing (ethersphere#1781) cmd/swarm: simplify testCluster.StartNewNodes (ethersphere#1777) build: increase golangci-lint deadline (ethersphere#1778) docker: ignore build/bin when copying files (ethersphere#1780) swap: fix and rename Peer.getLastSentCumulativePayout (ethersphere#1769) network/stream: more resilient TestNodesCorrectBinsDynamic (ethersphere#1776) network: Add Capabilities to Kademlia database (ethersphere#1713) network: add own address to KademliaInfo (ethersphere#1775) pss: Refactor. Step 2. Refactor forward cache (ethersphere#1742) all: configurable payment/disconnect thresholds (ethersphere#1729) network/stream/v2: more resilient TestNodesExchangeCorrectBinIndexes (ethersphere#1760) ...
This PR aims to refactor the "forward cache" part of PSS, which keeps track of known message digests during a specific amount of time.
The PR carves out this component as a generic
TTLSet( "set" understood as a collection of keys) which is a set that stores any key for a given time, and provides unit tests that deal with the expected result of adding a key to the set (.Add()), verifying whether a key is in the set (.Has()) and also if expired keys are cleaned timely.The PR also introduces a much needed concept of mocking the clock for testing: Even though the tests in this component check whether expired keys are cleaned from the set after a certain amount of time, it tests so with "virtual time", i.e., without using
time.Sleep(), which is always a source for either flaky tests or very slow ones: All tests of this package complete instantaneously and with a 100% coverage.