Repository: opensistemas-hub/osbrain
Pull request #262 (2018-03-05 16:41:25)

More robust stop-timer tests implementation
These tests were failing from time to time. Now they are more robust and fast (avoiding unnecessary time sleeps).

Peque: @ocaballeror Travis fails due to the linger test failing... :joy: (waiting for #260 to fix that). :stuck_out_tongue_winking_eye: 
Peque: @ocaballeror AppVeyor keeps failing on `ns.shutdown()` on test teardown. I try to update #259 with new occurrences. I say we fix all these CI issues before adding new features in this 0.7.0 cycle.
codecov[bot]: # [Codecov](https://codecov.io/gh/opensistemas-hub/osbrain/pull/262?src=pr&el=h1) Report
> Merging [#262](https://codecov.io/gh/opensistemas-hub/osbrain/pull/262?src=pr&el=desc) into [master](https://codecov.io/gh/opensistemas-hub/osbrain/commit/a5903c50d2ae793747d6ae22d106a40c5e603e80?src=pr&el=desc) will **decrease** coverage by `0.02%`.
> The diff coverage is `91.66%`.


[![Impacted file tree graph](https://codecov.io/gh/opensistemas-hub/osbrain/pull/262/graphs/tree.svg?token=LfvMoNOrOc&src=pr&width=650&height=150)](https://codecov.io/gh/opensistemas-hub/osbrain/pull/262?src=pr&el=tree)

```diff
@@           Coverage Diff            @@
##           master   #262      +/-   ##
========================================
- Coverage   99.03%    99%   -0.03%     
========================================
  Files          26     26              
  Lines        3528   3529       +1     
  Branches      251    252       +1     
========================================
  Hits         3494   3494              
  Misses         21     21              
- Partials       13     14       +1
```


| [Impacted Files](https://codecov.io/gh/opensistemas-hub/osbrain/pull/262?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [osbrain/tests/test\_timer.py](https://codecov.io/gh/opensistemas-hub/osbrain/pull/262/diff?src=pr&el=tree#diff-b3NicmFpbi90ZXN0cy90ZXN0X3RpbWVyLnB5) | `98.55% <91.66%> (-0.48%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/opensistemas-hub/osbrain/pull/262?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/opensistemas-hub/osbrain/pull/262?src=pr&el=footer). Last update [a5903c5...041bc31](https://codecov.io/gh/opensistemas-hub/osbrain/pull/262?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

ocaballeror: Yes! I never liked the arbitrary `time.sleep()`.

Let me test it a few times in Travis to make sure it works, you have my approval for everything else.
ocaballeror: This test looks like it could benefit from this as well: https://ci.appveyor.com/project/ocaballeror/osbrain/build/1.0.42/job/xbsqth4u9m1597s4#L761

Should we change it and include it in this PR?
Peque: Good catch with the typos! ^^

About the error in `osbrain/tests/test_agent_async_requests.py`, I had a look at it and I think it should be a separate issue. It is a good idea to change that, but if we do I'd rather re-think the whole `osbrain/tests/test_agent_async_requests.py` file, not just that particular test. So I say we merge this as-is if there are no other objections.

Patch:
diff --git a/osbrain/helper.py b/osbrain/helper.py
index fd052ae..b244466 100644
--- a/osbrain/helper.py
+++ b/osbrain/helper.py
@@ -114,7 +114,8 @@ def agent_dies(agent, nsproxy, timeout=1.):
     return True
 
 
-def attribute_match_all(attribute, length=None, data=None, value=None):
+def attribute_match_all(
+        attribute, length=None, data=None, value=None, endswith=None):
     """
     Check if an attribute matches all of the following specified conditions:
 
@@ -134,13 +135,16 @@ def attribute_match_all(attribute, length=None, data=None, value=None):
         If specified, the attribute must contain this value.
     value : anything, default is None
         If specified, the attribute must be this value.
+    endswith : sequence, default is None
+        If specified, the attribute must end with this sequence.
 
     Returns
     -------
     bool
         Whether the attribute matches any condition.
     """
-    assert length is not None or data is not None or value is not None, \
+    assert length is not None or data is not None or value is not None \
+        or endswith is not None, \
         'No condition passed! Will return True always...'
     if length is not None and len(attribute) < length:
         return False
@@ -148,11 +152,12 @@ def attribute_match_all(attribute, length=None, data=None, value=None):
         return False
     if value is not None and attribute != value:
         return False
+    if endswith is not None and attribute[-len(endswith):] != endswith:
+        return False
     return True
 
 
-def wait_agent_attr(agent, name='received', length=None, data=None, value=None,
-                    timeout=3):
+def wait_agent_attr(agent, name='received', timeout=3, **kwargs):
     """
     Wait for an agent's attribute to match all of the following specified
     conditions:
@@ -168,15 +173,11 @@ def wait_agent_attr(agent, name='received', length=None, data=None, value=None,
     agent : Proxy
         A proxy to the agent.
     name : str, default is `'received'`
-        Name of the agent's attribute to look for (should be a list).
-    length : int, default is None
-        If specified, wait until the attribute reaches this length or higher.
-    data : anything, default is None
-        If specified, wait until the attribute contains this element.
-    value : anything, default is None
-        If specified, wait until the attribute becomes this value.
+        Name of the agent's attribute to look for.
     timeout : float, default is 3
         After this number of seconds the function will return `False`.
+    kwargs : dict
+        Keyword arguments passed to `attribute_match_all` function.
 
     Returns
     -------
@@ -186,8 +187,7 @@ def wait_agent_attr(agent, name='received', length=None, data=None, value=None,
     t0 = time.time()
     while True:
         attribute = agent.get_attr(name)
-        if attribute_match_all(attribute, length=length, data=data,
-                               value=value):
+        if attribute_match_all(attribute, **kwargs):
             return True
         if time.time() - t0 > timeout:
             break


Test patch:
diff --git a/osbrain/tests/test_helper.py b/osbrain/tests/test_helper.py
index 2d704db..22734ac 100644
--- a/osbrain/tests/test_helper.py
+++ b/osbrain/tests/test_helper.py
@@ -118,25 +118,44 @@ def test_logger_received_count(agent_logger):
     assert logger_received(logger, message='foo.*bar') == 2
 
 
-@pytest.mark.parametrize('attribute,length,data,value,match', [
-    ([], 1, None, None, False),
-    ([], None, 1, None, False),
-    ([], None, None, [], True),
-    ([1, 2], 2, 2, None, True),
-    ([1, 2], 2, 3, None, False),
-    ([1, 2], 3, 2, None, False),
-    ({'foo'}, 1, None, None, True),
-    ({'foo'}, None, 'foo', None, True),
-    ({'foo'}, None, None, {'foo'}, True),
-    (42, None, None, 14, False),
-    (42, None, None, 42, True),
+@pytest.mark.parametrize('data,match', [
+    (dict(attribute=[], length=1), False),
+    (dict(attribute=[], data=1), False),
+    (dict(attribute=[], value=[]), True),
+    (dict(attribute=[1, 2], length=2, data=2), True),
+    (dict(attribute=[1, 2], length=2, data=3), False),
+    (dict(attribute=[1, 2], length=3, data=2), False),
+    (dict(attribute={'foo'}, length=1), True),
+    (dict(attribute={'foo'}, data='foo'), True),
+    (dict(attribute={'foo'}, value={'foo'}), True),
+    (dict(attribute=42, value=14), False),
+    (dict(attribute=42, value=42), True),
+    (dict(attribute=[1, 2, 3], endswith=[2, 3]), True),
+    (dict(attribute=[1, 2, 3], endswith=[3, 3]), False),
+    (dict(attribute='abcde', endswith='cde'), True),
+    (dict(attribute='abcde', endswith='ade'), False),
+], ids=[
+    'List length mismatch',
+    'Data not in list',
+    'List exact value',
+    'Data in list and list length',
+    'Data not in list with length match',
+    'Data in list with length mismatch',
+    'Dictionary attribute length',
+    'Key in dictionary',
+    'Dictionary exact value',
+    'Integer value mismatch',
+    'Integer value match',
+    'List ending with',
+    'List not ending with',
+    'String ending with',
+    'String not ending with',
 ])
-def test_attribute_match_all(attribute, length, data, value, match):
+def test_attribute_match_all(data, match):
     """
     Test `attribute_match_all` function.
     """
-    result = attribute_match_all(attribute, length=length, data=data,
-                                 value=value)
+    result = attribute_match_all(**data)
     assert result == match
 
 
diff --git a/osbrain/tests/test_timer.py b/osbrain/tests/test_timer.py
index 1aeaf52..f96762a 100644
--- a/osbrain/tests/test_timer.py
+++ b/osbrain/tests/test_timer.py
@@ -10,6 +10,7 @@ from osbrain.common import repeat
 from osbrain.helper import wait_agent_attr
 
 from common import nsproxy  # pragma: no flakes
+from common import append_received
 from common import set_received
 
 
@@ -159,70 +160,76 @@ def test_timer_each_stop_uuid(nsproxy):
     """
     Test a timer executed periodically and stopped by its UUID.
     """
-    def tick(agent):
-        agent.send('push', agent.count)
-        agent.count += 1
+    def tick(agent, message):
+        agent.send('push', message)
 
     sender = run_agent('sender')
-    receiver = run_agent('receiver')
+    receiver = run_agent('receiver', attributes={'received': []})
     addr = sender.bind('PUSH', alias='push')
-    receiver.connect(addr, handler=set_received)
+    receiver.connect(addr, handler=append_received)
 
     sender.set_attr(count=0)
-    uuid = sender.each(0.1, tick)
-    time.sleep(1)
-    assert abs(receiver.get_attr('received') - 10) <= 1
+    uuid = sender.each(0.01, tick, 'timer0')
+    sender.each(0.01, tick, 'timer1')
+    # Make sure both timers are running
+    assert wait_agent_attr(receiver, data='timer0')
+    assert wait_agent_attr(receiver, data='timer1')
+    # Stop one timer
     sender.stop_timer(uuid)
-    time.sleep(1)
-    assert abs(receiver.get_attr('received') - 10) <= 1
+    assert len(sender.list_timers()) == 1
     assert uuid not in sender.list_timers()
+    assert wait_agent_attr(receiver, endswith=['timer1'] * 10)
 
 
 def test_timer_each_stop_alias(nsproxy):
     """
     Test a timer executed periodically and stopped by an alias.
     """
-    def tick(agent):
-        agent.send('push', agent.count)
-        agent.count += 1
+    def tick(agent, message):
+        agent.send('push', message)
 
     sender = run_agent('sender')
-    receiver = run_agent('receiver')
+    receiver = run_agent('receiver', attributes={'received': []})
     addr = sender.bind('PUSH', alias='push')
-    receiver.connect(addr, handler=set_received)
+    receiver.connect(addr, handler=append_received)
 
     sender.set_attr(count=0)
-    sender.each(0.1, tick, alias='aliased_timer')
-    time.sleep(1)
-    assert abs(receiver.get_attr('received') - 10) <= 1
+    sender.each(0.01, tick, 'timer0', alias='aliased_timer')
+    sender.each(0.01, tick, 'timer1')
+    # Make sure both timers are running
+    assert wait_agent_attr(receiver, data='timer0')
+    assert wait_agent_attr(receiver, data='timer1')
+    # Stop one timer
     sender.stop_timer('aliased_timer')
-    time.sleep(1)
-    assert abs(receiver.get_attr('received') - 10) <= 1
+    assert len(sender.list_timers()) == 1
     assert 'aliased_timer' not in sender.list_timers()
+    assert wait_agent_attr(receiver, endswith=['timer1'] * 10)
 
 
 def test_stop_all_timers(nsproxy):
     """
     Calling `stop_all_timers()` should stop all currently running timers.
     """
-    def tick(agent):
-        agent.send('push', agent.count)
-        agent.count += 1
+    def tick(agent, message):
+        agent.send('push', message)
 
     sender = run_agent('sender')
-    receiver = run_agent('receiver')
+    receiver = run_agent('receiver', attributes={'received': []})
     addr = sender.bind('PUSH', alias='push')
-    receiver.connect(addr, handler=set_received)
+    receiver.connect(addr, handler=append_received)
 
     sender.set_attr(count=0)
-    sender.each(0.1, tick, alias='timer0')
-    sender.each(0.1, tick, alias='timer1')
-    time.sleep(1)
-    assert abs(receiver.get_attr('received') - 20) <= 1
+    sender.each(0.01, tick, 'timer0')
+    sender.each(0.01, tick, 'timer1')
+    # Make sure both timers are running
+    assert wait_agent_attr(receiver, data='timer0')
+    assert wait_agent_attr(receiver, data='timer1')
+    # Stop all timers
     sender.stop_all_timers()
-    time.sleep(1)
-    assert abs(receiver.get_attr('received') - 20) <= 1
     assert len(sender.list_timers()) == 0
+    # Run a new timer (only this one should be running now)
+    sender.each(0.01, tick, 'timer3')
+    assert wait_agent_attr(receiver, endswith=['timer3'] * 10)
 
 
 def test_timer_after(nsproxy):