Refactor Federation Watcher Tests #314
Conversation
apancorb
force-pushed
the
refactor/federation_watcher_tests
branch
from
471a59e to
96b6861
Compare
| federationWatcher.addListener(new FederationWatcher.Listener() { | ||
| @Override | ||
| public void onActiveFederationChange(Optional<Federation> oldFederation, Federation newFederation) { | ||
| assertEquals(Optional.empty(), oldFederation); | ||
| assertEquals(FIRST_FEDERATION, newFederation); | ||
| activeCalls.incrementAndGet(); | ||
| } | ||
|
|
||
| @Override | ||
| public void onRetiringFederationChange(Optional<Federation> oldFederation, Optional<Federation> newFederation) { | ||
| retiringCalls.incrementAndGet(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
wdyt about extracting this to a method? I see it duplicated a couple of times
There was a problem hiding this comment.
For each test the anoymous class implementation is a bit different if you notice
|
|
||
| when(federationProvider.getActiveFederationAddress()).thenReturn(federation1.getAddress()); | ||
| when(federationProvider.getRetiringFederationAddress()).thenReturn(Optional.empty()); | ||
| void whenNoActiveFederationChange_shouldNotTriggerActiveOrRetiringFederationChange() throws Exception { |
There was a problem hiding this comment.
| void whenNoActiveFederationChange_shouldNotTriggerActiveOrRetiringFederationChange() throws Exception { | |
| void onBestBlock_whenNoActiveFederationChange_shouldNotTriggerActiveOrRetiringFederationChange() throws Exception { |
wdyt? To be clear about the method we are calling
There was a problem hiding this comment.
The thing is that we are not testing the onBestBlock implementation, just a side effect of calling it. So it could confuse the reader if we preface it as such.
| verify(federationProvider, times(1)).getActiveFederation(); | ||
|
|
||
| // Assert | ||
| assertEquals(1, activeCalls.get()); |
There was a problem hiding this comment.
wasn't asserting the active calls were 2 before?
There was a problem hiding this comment.
Yes this was being done before. It was testing the same "code" twice since the listeners have the same implementation. So it leads to redundancy, not sure why it was designed like this as a whole :)
| var activeCalls = new AtomicInteger(0); | ||
| var retiringCalls = new AtomicInteger(0); |
There was a problem hiding this comment.
wdyt about a global setUp to reset these values?
There was a problem hiding this comment.
Seems to overcomplicate things for a local counter variable
|
marcos-iov
merged commit c95edea
into
feat/powpeg_validation_protocol-phase3
Refactor Federation Watcher Tests
Refactor Federation Watcher Tests
Refactor Federation Watcher Tests
Refactor Federation Watcher Tests
Summary
Refactor existing Federation Watcher Tests so that when we introduce the new logic for signing the svp spend transaction on proposed federation change we are ready.
Motivation
https://github.com/rsksmart/RSKIPs/blob/master/IPs/RSKIP419.md