From 5e392fdcf1a9922ebd65e53054be6f919fb48268 Mon Sep 17 00:00:00 2001 From: Sergey Khoroshavin Date: Mon, 12 Mar 2018 18:27:54 +0300 Subject: [PATCH 1/6] Refactored test_delay in test_stasher Signed-off-by: Sergey Khoroshavin --- plenum/test/test_stasher.py | 37 ++++++++++++++----------------------- 1 file changed, 14 insertions(+), 23 deletions(-) diff --git a/plenum/test/test_stasher.py b/plenum/test/test_stasher.py index 254f87bc5a..0e2274ffbd 100644 --- a/plenum/test/test_stasher.py +++ b/plenum/test/test_stasher.py @@ -8,41 +8,32 @@ @pytest.mark.skipif('sys.platform == "win32"', reason='SOV-457') def test_delay(): - x = deque() - s = Stasher(x, "my-stasher") - x.append(1) - x.append(2) - x.append(3) + q = deque([1,2,3]) + s = Stasher(q, "my-stasher") - def delayTwos(item): + def delay_twos(item): if item == 2: return 2 - s.delay(delayTwos) - + # Check that relevant items are stashed from deque + s.delay(delay_twos) s.process() - r1 = x.popleft() - assert r1 == 1 - - r2 = x.popleft() - assert r2 == 3 + assert list(q) == [1, 3] - with pytest.raises(IndexError): - x.popleft() + # Pretend that we processed items that are not stashed + q.clear() + # Check that nothing happened after one second time.sleep(1) s.process() + assert list(q) == [] - with pytest.raises(IndexError): - x.popleft() - + # Check that stashed items returned to deque after one more second time.sleep(1) s.process() + assert list(q) == [2] - r3 = x.popleft() - assert r3 == 2 - - x.append(2) + # Check that items are no longer stashed when delays are reset s.resetDelays() s.process() - assert 2 == x.popleft() + assert list(q) == [2] From 01da7f03002a40452a13df08f35e350db6f834bc Mon Sep 17 00:00:00 2001 From: Sergey Khoroshavin Date: Mon, 12 Mar 2018 19:34:35 +0300 Subject: [PATCH 2/6] Initial broken implementation of stasher delay_rules context Signed-off-by: Sergey Khoroshavin --- plenum/test/stasher.py | 12 +++++++++ plenum/test/test_stasher.py | 51 ++++++++++++++++++++++++++++++++----- 2 files changed, 57 insertions(+), 6 deletions(-) diff --git a/plenum/test/stasher.py b/plenum/test/stasher.py index 255356d006..fc6c585227 100644 --- a/plenum/test/stasher.py +++ b/plenum/test/stasher.py @@ -1,4 +1,5 @@ import time +from contextlib import contextmanager from stp_core.common.log import getlogger @@ -108,3 +109,14 @@ def force_unstash(self, *names): def reset_delays_and_process_delayeds(self, *names): self.resetDelays(*names) self.force_unstash(*names) + + +@contextmanager +def delay_rules(stasher, delayer): + stasher.delay(delayer) + yield + try: + stasher.reset_delays_and_process_delayeds(delayer.__name__) + except: + pass + diff --git a/plenum/test/test_stasher.py b/plenum/test/test_stasher.py index 0e2274ffbd..748d216196 100644 --- a/plenum/test/test_stasher.py +++ b/plenum/test/test_stasher.py @@ -3,18 +3,24 @@ import pytest -from plenum.test.stasher import Stasher +from plenum.test.stasher import Stasher, delay_rules + + +def delay_twos(item): + if item == 2: + return 2 + + +def delay_threes(item): + if item == 3: + return 2 @pytest.mark.skipif('sys.platform == "win32"', reason='SOV-457') def test_delay(): - q = deque([1,2,3]) + q = deque([1, 2, 3]) s = Stasher(q, "my-stasher") - def delay_twos(item): - if item == 2: - return 2 - # Check that relevant items are stashed from deque s.delay(delay_twos) s.process() @@ -37,3 +43,36 @@ def delay_twos(item): s.resetDelays() s.process() assert list(q) == [2] + + +def test_delay_rules_enable_delays_on_entry_and_disables_them_on_exit(): + s = Stasher(deque()) + + with delay_rules(s, delay_twos): + assert delay_twos in s.delayRules + + assert delay_twos not in s.delayRules + + +def test_delay_rules_dont_touch_other_delays(): + s = Stasher(deque()) + s.delay(delay_threes) + + with delay_rules(s, delay_twos): + assert delay_threes in s.delayRules + + assert delay_threes in s.delayRules + + +def test_delay_rules_return_delayed_items_to_list_on_exit(): + q = deque([1, 2, 3]) + s = Stasher(q) + s.delay(delay_threes) + + with delay_rules(s, delay_twos): + s.process() + assert 2 not in q + assert 3 not in q + + assert 2 in q + assert 3 not in q From 84f24696e2befe8d3669578ed1bd18cf5679986e Mon Sep 17 00:00:00 2001 From: Sergey Khoroshavin Date: Mon, 12 Mar 2018 20:31:34 +0300 Subject: [PATCH 3/6] Possibly breaking changes in Stasher to fix context manager tests Signed-off-by: Sergey Khoroshavin --- plenum/test/stasher.py | 28 ++++++++++++++-------------- plenum/test/test_stasher.py | 14 ++++++++++++++ 2 files changed, 28 insertions(+), 14 deletions(-) diff --git a/plenum/test/stasher.py b/plenum/test/stasher.py index fc6c585227..2bbf85f40a 100644 --- a/plenum/test/stasher.py +++ b/plenum/test/stasher.py @@ -1,10 +1,13 @@ import time +from collections import namedtuple from contextlib import contextmanager from stp_core.common.log import getlogger logger = getlogger() +StasherDelayed = namedtuple('StasherDelayed', 'item timestamp rule') + class Stasher: def __init__(self, queue, name: str = None): @@ -43,7 +46,7 @@ def stashAll(self, age): logger.debug("{} stashing message {} for " "{} seconds". format(self.name, rx, secondsToDelay)) - self.delayeds.append((age + secondsToDelay, rx)) + self.delayeds.append(StasherDelayed(item=rx, timestamp=age + secondsToDelay, rule=tester.__name__)) self.queue.remove(rx) def unstashAll(self, age, *names, ignore_age_check=False): @@ -60,18 +63,18 @@ def unstashAll(self, age, *names, ignore_age_check=False): # This is in-efficient as `ignore_age_check` wont change during loop # but its ok since its a testing util. if ignore_age_check or ( - names and d[1][0].__name__ in names) or age >= d[0]: + names and d.rule in names) or age >= d.timestamp: if ignore_age_check: msg = '(forced)' - elif names and d[1][0].__name__ in names: - msg = '({} present in {})'.format(d[1][0].__name__, names) + elif names and d.rule in names: + msg = '({} present in {})'.format(d.rule, names) else: msg = '({:.0f} milliseconds overdue)'.format( - (age - d[0]) * 1000) + (age - d.timestamp) * 1000) logger.debug( "{} unstashing message {} {}". - format(self.name, d[1], msg)) - self.queue.appendleft(d[1]) + format(self.name, d.item, msg)) + self.queue.appendleft(d.item) to_remove.append(idx) unstashed += 1 @@ -112,11 +115,8 @@ def reset_delays_and_process_delayeds(self, *names): @contextmanager -def delay_rules(stasher, delayer): - stasher.delay(delayer) +def delay_rules(stasher, *delayers): + for d in delayers: + stasher.delay(d) yield - try: - stasher.reset_delays_and_process_delayeds(delayer.__name__) - except: - pass - + stasher.reset_delays_and_process_delayeds(*(d.__name__ for d in delayers)) diff --git a/plenum/test/test_stasher.py b/plenum/test/test_stasher.py index 748d216196..70390aa1fb 100644 --- a/plenum/test/test_stasher.py +++ b/plenum/test/test_stasher.py @@ -71,8 +71,22 @@ def test_delay_rules_return_delayed_items_to_list_on_exit(): with delay_rules(s, delay_twos): s.process() + assert 1 in q assert 2 not in q assert 3 not in q + assert 1 in q assert 2 in q assert 3 not in q + + +def test_delay_rules_can_use_multiple_delayers(): + s = Stasher(deque()) + + with delay_rules(s, delay_twos, delay_threes): + assert delay_twos in s.delayRules + assert delay_threes in s.delayRules + + assert delay_twos not in s.delayRules + assert delay_threes not in s.delayRules + From dfe81cdeea1cd71315bc12f444801b3c66d53529 Mon Sep 17 00:00:00 2001 From: Sergey Khoroshavin Date: Tue, 13 Mar 2018 11:34:18 +0300 Subject: [PATCH 4/6] Documented changes in stasher Signed-off-by: Sergey Khoroshavin --- plenum/test/stasher.py | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/plenum/test/stasher.py b/plenum/test/stasher.py index 2bbf85f40a..f66c527f79 100644 --- a/plenum/test/stasher.py +++ b/plenum/test/stasher.py @@ -23,6 +23,10 @@ def delay(self, tester): :param tester: a callable that takes as an argument the item from the queue and returns a number of seconds it should be delayed + + Note: current reliance on tester.__name__ to remove particular + delay rules could lead to problems when adding testers with + same function but different parameters. """ logger.debug("{} adding delay for {}".format(self.name, tester)) self.delayRules.add(tester) @@ -110,12 +114,31 @@ def force_unstash(self, *names): return self.unstashAll(0, *names) def reset_delays_and_process_delayeds(self, *names): + """ + Remove delay rules and unstash related messages. + + :param names: list of delay functions names to unstash + + Note that original implementation made an assumption that messages are tuples + and relied on first element __name__ to find messages to unstash, but new one + explicitly stores name of delay rule function when stashing messages. Also + most delay rule functions override their __name__ to match delayed message. + While usages looking like reset_delays_and_process_delayeds(COMMIT) won't break + as long as last assumption holds true it's still recommended to consider using + new context manager where applicable to reduce potential errors in tests. + """ self.resetDelays(*names) self.force_unstash(*names) @contextmanager def delay_rules(stasher, *delayers): + """ + Context manager to add delay rules to stasher(s) on entry and clean everything up on exit. + + :param stasher: Instance of Stasher or iterable over instances of stasher + :param delayers: Delay rule functions to be added to stashers + """ for d in delayers: stasher.delay(d) yield From 64c75aaecf741eea34a9100c7df21f86f210b282 Mon Sep 17 00:00:00 2001 From: Sergey Khoroshavin Date: Tue, 13 Mar 2018 11:56:46 +0300 Subject: [PATCH 5/6] Improved delay_rules implementation Signed-off-by: Sergey Khoroshavin --- plenum/test/stasher.py | 17 ++++++++++++++--- plenum/test/test_stasher.py | 30 ++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 3 deletions(-) diff --git a/plenum/test/stasher.py b/plenum/test/stasher.py index f66c527f79..b6b83c201e 100644 --- a/plenum/test/stasher.py +++ b/plenum/test/stasher.py @@ -139,7 +139,18 @@ def delay_rules(stasher, *delayers): :param stasher: Instance of Stasher or iterable over instances of stasher :param delayers: Delay rule functions to be added to stashers """ - for d in delayers: - stasher.delay(d) + try: + stashers = [s for s in stasher] + except TypeError: + stashers = [stasher] + + for s in stashers: + if not isinstance(s, Stasher): + raise TypeError("expected Stasher or Iterable[Stasher] as a first argument") + + for s in stashers: + for d in delayers: + s.delay(d) yield - stasher.reset_delays_and_process_delayeds(*(d.__name__ for d in delayers)) + for s in stashers: + s.reset_delays_and_process_delayeds(*(d.__name__ for d in delayers)) diff --git a/plenum/test/test_stasher.py b/plenum/test/test_stasher.py index 70390aa1fb..d36d651ad3 100644 --- a/plenum/test/test_stasher.py +++ b/plenum/test/test_stasher.py @@ -90,3 +90,33 @@ def test_delay_rules_can_use_multiple_delayers(): assert delay_twos not in s.delayRules assert delay_threes not in s.delayRules + +def test_delay_rules_can_use_multiple_stashers(): + s1 = Stasher(deque()) + s2 = Stasher(deque()) + + with delay_rules([s1, s2], delay_twos): + assert delay_twos in s1.delayRules + assert delay_twos in s2.delayRules + + assert delay_twos not in s1.delayRules + assert delay_twos not in s2.delayRules + + +def test_delay_rules_can_use_generator_expressions(): + stashers = [Stasher(deque(), name="{}".format(i)) for i in range(3)] + + with delay_rules((s for s in stashers if s.name != "1"), delay_twos): + assert delay_twos in stashers[0].delayRules + assert delay_twos not in stashers[1].delayRules + assert delay_twos in stashers[2].delayRules + + assert delay_twos not in stashers[0].delayRules + assert delay_twos not in stashers[1].delayRules + assert delay_twos not in stashers[2].delayRules + + +def test_delay_rules_throws_when_given_stashers_of_wrong_type(): + with pytest.raises(TypeError): + with delay_rules(1, delay_twos): + pass \ No newline at end of file From 91723e7a52a574c249c9b846c2b6f66fce7d57eb Mon Sep 17 00:00:00 2001 From: Sergey Khoroshavin Date: Tue, 13 Mar 2018 12:13:02 +0300 Subject: [PATCH 6/6] Example use of new delay_rules context manager + misc cleanups Signed-off-by: Sergey Khoroshavin --- plenum/test/delayers.py | 33 ++++++----- .../test_timestamp_post_view_change.py | 58 ++++++++----------- plenum/test/test_stasher.py | 12 +++- 3 files changed, 52 insertions(+), 51 deletions(-) diff --git a/plenum/test/delayers.py b/plenum/test/delayers.py index aa1217cd66..f1da56fa9d 100644 --- a/plenum/test/delayers.py +++ b/plenum/test/delayers.py @@ -13,6 +13,7 @@ from plenum.common.util import getCallableName from plenum.test.test_client import TestClient +DEFAULT_DELAY = 600 def delayer(seconds, op, senderFilter=None, instFilter: int = None): def inner(rx): @@ -74,83 +75,83 @@ def inner(action_pair): return inner -def nom_delay(delay: float, inst_id=None, sender_filter: str=None): +def nom_delay(delay: float = DEFAULT_DELAY, inst_id=None, sender_filter: str=None): # Delayer of NOMINATE requests return delayerMsgTuple( delay, Nomination, instFilter=inst_id, senderFilter=sender_filter) -def prim_delay(delay: float, inst_id=None, sender_filter: str=None): +def prim_delay(delay: float = DEFAULT_DELAY, inst_id=None, sender_filter: str=None): # Delayer of PRIMARY requests return delayerMsgTuple( delay, Primary, instFilter=inst_id, senderFilter=sender_filter) -def rel_delay(delay: float, inst_id=None, sender_filter: str=None): +def rel_delay(delay: float = DEFAULT_DELAY, inst_id=None, sender_filter: str=None): # Delayer of REELECTION requests return delayerMsgTuple( delay, Reelection, instFilter=inst_id, senderFilter=sender_filter) -def ppgDelay(delay: float, sender_filter: str=None): +def ppgDelay(delay: float = DEFAULT_DELAY, sender_filter: str=None): # Delayer of PROPAGATE requests return delayerMsgTuple(delay, Propagate, senderFilter=sender_filter) -def ppDelay(delay: float, instId: int=None, sender_filter: str=None): +def ppDelay(delay: float = DEFAULT_DELAY, instId: int=None, sender_filter: str=None): # Delayer of PRE-PREPARE requests from a particular instance return delayerMsgTuple(delay, PrePrepare, instFilter=instId, senderFilter=sender_filter) -def pDelay(delay: float, instId: int=None, sender_filter: str=None): +def pDelay(delay: float = DEFAULT_DELAY, instId: int=None, sender_filter: str=None): # Delayer of PREPARE requests from a particular instance return delayerMsgTuple( delay, Prepare, instFilter=instId, senderFilter=sender_filter) -def cDelay(delay: float, instId: int=None, sender_filter: str=None): +def cDelay(delay: float = DEFAULT_DELAY, instId: int=None, sender_filter: str=None): # Delayer of COMMIT requests from a particular instance return delayerMsgTuple( delay, Commit, instFilter=instId, senderFilter=sender_filter) -def icDelay(delay: float): +def icDelay(delay: float = DEFAULT_DELAY): # Delayer of INSTANCE-CHANGE requests return delayerMsgTuple(delay, InstanceChange) -def vcd_delay(delay: float): +def vcd_delay(delay: float = DEFAULT_DELAY): # Delayer of VIEW_CHANGE_DONE requests return delayerMsgTuple(delay, ViewChangeDone) -def lsDelay(delay: float): +def lsDelay(delay: float = DEFAULT_DELAY): # Delayer of LEDGER_STATUSES requests return delayerMsgTuple(delay, LedgerStatus) -def cpDelay(delay: float): +def cpDelay(delay: float = DEFAULT_DELAY): # Delayer of CONSISTENCY_PROOFS requests return delayerMsgTuple(delay, ConsistencyProof) -def cqDelay(delay: float): +def cqDelay(delay: float = DEFAULT_DELAY): # Delayer of CATCHUP_REQ requests return delayerMsgTuple(delay, CatchupReq) -def cr_delay(delay: float): +def cr_delay(delay: float = DEFAULT_DELAY): # Delayer of CATCHUP_REP requests return delayerMsgTuple(delay, CatchupRep) -def req_delay(delay: float): +def req_delay(delay: float = DEFAULT_DELAY): # Delayer of Request requests return delayerMsgTuple(delay, Request) -def msg_req_delay(delay: float, types_to_delay: List=None): +def msg_req_delay(delay: float = DEFAULT_DELAY, types_to_delay: List=None): # Delayer of MessageReq messages def specific_msgs(msg): if isinstance( @@ -162,7 +163,7 @@ def specific_msgs(msg): return specific_msgs -def msg_rep_delay(delay: float, types_to_delay: List=None): +def msg_rep_delay(delay: float = DEFAULT_DELAY, types_to_delay: List=None): # Delayer of MessageRep messages def specific_msgs(msg): if isinstance( diff --git a/plenum/test/node_request/test_timestamp/test_timestamp_post_view_change.py b/plenum/test/node_request/test_timestamp/test_timestamp_post_view_change.py index 53538e69b7..eac7b9c503 100644 --- a/plenum/test/node_request/test_timestamp/test_timestamp_post_view_change.py +++ b/plenum/test/node_request/test_timestamp/test_timestamp_post_view_change.py @@ -7,6 +7,7 @@ from plenum.test.test_node import ensureElectionsDone, getNonPrimaryReplicas from plenum.test.view_change.helper import ensure_view_change from plenum.test.helper import sdk_send_random_and_check, sdk_send_random_requests +from plenum.test.stasher import delay_rules from plenum.test.delayers import icDelay Max3PCBatchSize = 4 @@ -52,40 +53,31 @@ def test_new_primary_has_wrong_clock(tconf, looper, txnPoolNodeSet, old_view_no = txnPoolNodeSet[0].viewNo - # Delay parameters - malicious_batch_count = 5 - malicious_batch_interval = 2 - instance_change_delay = 1.5 * malicious_batch_count * malicious_batch_interval - # Delay instance change so view change doesn't happen in the middle of this test - for node in txnPoolNodeSet: - node.nodeIbStasher.delay(icDelay(instance_change_delay)) - - # Requests are sent - for _ in range(malicious_batch_count): - sdk_send_random_requests(looper, - sdk_pool_handle, - sdk_wallet_client, - count=2) - looper.runFor(malicious_batch_interval) - - def chk(): - for node in txnPoolNodeSet: - assert node.viewNo == old_view_no - - for node in [n for n in txnPoolNodeSet if n != faulty_node]: - # Each non faulty node raises suspicion - assert get_timestamp_suspicion_count(node) > susp_counts[node.name] - # Ledger does not change - assert node.domainLedger.size == ledger_sizes[node.name] - - assert faulty_node.domainLedger.size == ledger_sizes[faulty_node.name] - - looper.run(eventually(chk, retryWait=1)) - - # Clear delays - for node in txnPoolNodeSet: - node.nodeIbStasher.reset_delays_and_process_delayeds() + stashers = (n.nodeIbStasher for n in txnPoolNodeSet) + with delay_rules(stashers, icDelay()): + # Requests are sent + for _ in range(5): + sdk_send_random_requests(looper, + sdk_pool_handle, + sdk_wallet_client, + count=2) + looper.runFor(2) + + def chk(): + for node in txnPoolNodeSet: + assert node.viewNo == old_view_no + + for node in [n for n in txnPoolNodeSet if n != faulty_node]: + # Each non faulty node raises suspicion + assert get_timestamp_suspicion_count(node) > susp_counts[node.name] + # Ledger does not change + assert node.domainLedger.size == ledger_sizes[node.name] + + assert faulty_node.domainLedger.size == ledger_sizes[faulty_node.name] + + looper.run(eventually(chk, retryWait=1)) + # Eventually another view change happens looper.run(eventually(checkViewNoForNodes, txnPoolNodeSet, old_view_no + 1, diff --git a/plenum/test/test_stasher.py b/plenum/test/test_stasher.py index d36d651ad3..1970228577 100644 --- a/plenum/test/test_stasher.py +++ b/plenum/test/test_stasher.py @@ -116,7 +116,15 @@ def test_delay_rules_can_use_generator_expressions(): assert delay_twos not in stashers[2].delayRules -def test_delay_rules_throws_when_given_stashers_of_wrong_type(): +def test_delay_rules_raise_type_error_when_given_stashers_of_wrong_type(): with pytest.raises(TypeError): with delay_rules(1, delay_twos): - pass \ No newline at end of file + pass + + with pytest.raises(TypeError): + with delay_rules([1, 2], delay_twos): + pass + + with pytest.raises(TypeError): + with delay_rules(range(3), delay_twos): + pass