Added Tests for class PathStore in lib/path_store.py #214

prateshg · 2015-06-29T13:43:58Z

General Comment
<img border=0 src='https://avatars.githubusercontent.com/u/12234672?v=3' height=16 width=16'> @kormat Ready for review
<img border=0 src='https://avatars.githubusercontent.com/u/6919822?v=3' height=16 width=16'> @prateshg - there's ~5 lines missing from test coverage in this file. If this loads for you, you should be able to see them: https://circle-artifacts.com/gh/netsec-ethz/scion/613/artifacts/0/tmp/circle-artifacts.ylUPwNl/coverage/lib_path_store.html (it's taking ages for me). Otherwise run './scion.sh coverage lib_path_store_test' and then look at the html report that's linked at the bottom of the output.
The affected code is not part of what you're testing here, so i'm happy for you to make a new PR to fix that, and i'll merge this one.
<img border=0 src='https://avatars.githubusercontent.com/u/12234672?v=3' height=16 width=16'> I just looked at those 5 lines. The first 2 are because of the test generator that is used. The next 3 are for except in (try/except) so I am not sure do I raise an exception for testing this?
<img border=0 src='https://avatars.githubusercontent.com/u/12234672?v=3' height=16 width=16'> @kormat you can merge this
<img border=0 src='https://avatars.githubusercontent.com/u/6919822?v=3' height=16 width=16'> @prateshg - The test generator has a bug in it, and i'm surprised it didn't fail :(
pth_pol.property_ranges['GuaranteedBandwidth'].extend(range_)
should be:
pth_pol.property_ranges[key].extend(range_)
<img border=0 src='https://avatars.githubusercontent.com/u/6919822?v=3' height=16 width=16'> And yes, for from_file, you should use a test generator to raise all 3 exceptions.
<img border=0 src='https://avatars.githubusercontent.com/u/12234672?v=3' height=16 width=16'> Well it didn't fail because of the almost equal
<img border=0 src='https://avatars.githubusercontent.com/u/6919822?v=3' height=16 width=16'> Maybe we're looking at somewhere different, but https://github.com/netsec-ethz/scion/blob/master/test/lib_path_store_test.py#L200 just uses eq_
<img border=0 src='https://avatars.githubusercontent.com/u/6919822?v=3' height=16 width=16'> Ahh. The return of check_property_ranges defaults to True
File: test/lib_path_store_test.py:L361-706
<img border=0 src='https://avatars.githubusercontent.com/u/6919822?v=3' height=16 width=16'> This name is a duplicate
File: test/lib_path_store_test.py:L361-706
<img border=0 src='https://avatars.githubusercontent.com/u/6919822?v=3' height=16 width=16'> Need a test for when check_filters returns False
<img border=0 src='https://avatars.githubusercontent.com/u/6919822?v=3' height=16 width=16'> Oops, i missed test_filters
<img border=0 src='https://avatars.githubusercontent.com/u/6919822?v=3' height=16 width=16'> 👍
<img border=0 src='https://avatars.githubusercontent.com/u/12234672?v=3' height=16 width=16'> There is one for when check_filters return false "test_filters"
File: test/lib_path_store_test.py:L361-706
<img border=0 src='https://avatars.githubusercontent.com/u/6919822?v=3' height=16 width=16'> Need a check that get_n_peer_links() was called on each candidate.
File: test/lib_path_store_test.py:L361-706
<img border=0 src='https://avatars.githubusercontent.com/u/6919822?v=3' height=16 width=16'> Check that get_n_hops is called
File: test/lib_path_store_test.py:L361-706
<img border=0 src='https://avatars.githubusercontent.com/u/6919822?v=3' height=16 width=16'> Check that get_timestamp is called
<img border=0 src='https://avatars.githubusercontent.com/u/12234672?v=3' height=16 width=16'> Fixed
File: test/lib_path_store_test.py:L361-706
<img border=0 src='https://avatars.githubusercontent.com/u/6919822?v=3' height=16 width=16'> Having setUp/tearDown methods that aren't used by all test cases (and in fact whose work is partially duplicated by a test case) isn't a good approach. I would instead make a _setup method like:

def _setup(self, attrs=None):
def_attrs = {'history_limit': 3}
if attrs:
def_attrs.update(attrs)
path_policy = MagicMock(spec_set=def_attrs.keys())
for k, v in def_attrs.items():
setattr(path_policy, k, v)
return path_policy

<img border=0 src='https://avatars.githubusercontent.com/u/6919822?v=3' height=16 width=16'> And then call that from each test method, passing in a different attribute dictionary as necessary.
<img border=0 src='https://avatars.githubusercontent.com/u/12234672?v=3' height=16 width=16'> Fixed
(ping)
File: test/lib_path_store_test.py:L361-710
<img border=0 src='https://avatars.githubusercontent.com/u/6919822?v=3' height=16 width=16'> This is a bit weird. It would be simpler to just do:

pth_str.best_paths_history = []
pth_str.best_paths_history.append([MagicMock(spec_set=['pcb'] for i in range(5)])

<img border=0 src='https://avatars.githubusercontent.com/u/12234672?v=3' height=16 width=16'> Fixed

prateshg · 2015-06-29T13:47:36Z

@kormat Ready for review

kormat · 2015-06-29T14:52:30Z

test/lib_path_store_test.py

+        ntools.eq_(pth_str.best_paths_history, "best_paths_history")
+
+
+class TestPathStoreInit(object):


This name is a duplicate

kormat · 2015-06-29T15:07:46Z

test/lib_path_store_test.py

+class TestPathStoreInit(object):
+    """
+    Unit tests for lib.path_store.PathStore.__init__
+    """


Need a test for when check_filters returns False

Oops, i missed test_filters

There is one for when check_filters return false "test_filters"

kormat · 2015-06-29T15:30:54Z

test/lib_path_store_test.py

+            pcb = MagicMock(spec_set=['get_n_hops'])
+            pcb.get_n_hops.return_value = 2*i + 2
+            pth_str.candidates[i].pcb = pcb
+        pth_str._update_all_hops_length()


Check that get_n_hops is called

kormat · 2015-06-30T12:40:07Z

test/lib_path_store_test.py

+    def test_basic(self):
+        pth_str = PathStore(self._setup())
+        pth_str.best_paths_history = [1]
+        pth_str.best_paths_history[0] = [MagicMock(spec_set=['pcb'])


This is a bit weird. It would be simpler to just do:

pth_str.best_paths_history = [] pth_str.best_paths_history.append([MagicMock(spec_set=['pcb'] for i in range(5)])

kormat · 2015-06-30T12:51:27Z

@prateshg - there's ~5 lines missing from test coverage in this file. If this loads for you, you should be able to see them: https://circle-artifacts.com/gh/netsec-ethz/scion/613/artifacts/0/tmp/circle-artifacts.ylUPwNl/coverage/lib_path_store.html (it's taking ages for me). Otherwise run './scion.sh coverage lib_path_store_test' and then look at the html report that's linked at the bottom of the output.

The affected code is not part of what you're testing here, so i'm happy for you to make a new PR to fix that, and i'll merge this one.

prateshg · 2015-06-30T13:00:05Z

I just looked at those 5 lines. The first 2 are because of the test generator that is used that I will fix. The next 3 are for except in (try/except) so I am not sure do I raise an exception for testing this?

prateshg · 2015-06-30T13:04:23Z

@kormat you can merge this

Added Tests for class PathStore in lib/path_store.py

kormat · 2015-06-30T13:07:53Z

@prateshg - The test generator has a bug in it, and i'm surprised it didn't fail :(
pth_pol.property_ranges['GuaranteedBandwidth'].extend(range_)
should be:
pth_pol.property_ranges[key].extend(range_)

kormat · 2015-06-30T13:08:42Z

And yes, for from_file, you should use a test generator to raise all 3 exceptions.

kormat · 2015-06-30T13:24:43Z

Maybe we're looking at somewhere different, but https://github.com/netsec-ethz/scion/blob/master/test/lib_path_store_test.py#L200 just uses eq_

kormat · 2015-06-30T13:25:42Z

Ahh. The return of check_property_ranges defaults to True

Added Tests for class PathStore in lib/path_store.py

prateshg added 5 commits June 26, 2015 16:51

Added Tests for PathStore class

aeb962d

Merge branch 'master' of https://github.com/netsec-ethz/scion

7a67482

Merge branch 'master' of https://github.com/netsec-ethz/scion

d124f37

Added missing tests

7832090

Update path_store.py

6b6d9fe

kormat mentioned this pull request Jun 29, 2015

Unit testing master issue #129

Closed

kormat reviewed Jun 29, 2015
View reviewed changes

Removed Duplicate Name

3c443ee

kormat reviewed Jun 29, 2015
View reviewed changes

Addressed Comments

1354e71

kormat reviewed Jun 29, 2015
View reviewed changes

Used _setup

6bdf83a

kormat reviewed Jun 30, 2015
View reviewed changes

Addressed Comments

81fde2e

kormat added a commit that referenced this pull request Jun 30, 2015

Merge pull request #214 from prateshg/master

ac30ed8

Added Tests for class PathStore in lib/path_store.py

kormat merged commit ac30ed8 into scionproto:master Jun 30, 2015

kormat mentioned this pull request Jun 30, 2015

minor issues with lib_path_store_test.py #216

Closed

FR4NK-W pushed a commit to FR4NK-W/osourced-scion that referenced this pull request Feb 14, 2017

Merge pull request scionproto#214 from prateshg/master

aa69c71

Added Tests for class PathStore in lib/path_store.py

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Tests for class PathStore in lib/path_store.py #214

Added Tests for class PathStore in lib/path_store.py #214

prateshg commented Jun 29, 2015

prateshg commented Jun 29, 2015

kormat Jun 29, 2015

kormat Jun 30, 2015

kormat Jun 29, 2015

kormat Jun 29, 2015

kormat Jun 29, 2015

prateshg Jun 29, 2015

kormat Jun 30, 2015

kormat Jun 29, 2015

kormat Jun 30, 2015

kormat Jun 30, 2015

prateshg Jun 30, 2015

kormat Jun 30, 2015

kormat commented Jun 30, 2015

prateshg commented Jun 30, 2015

prateshg commented Jun 30, 2015

kormat commented Jun 30, 2015

kormat commented Jun 30, 2015

kormat commented Jun 30, 2015

kormat commented Jun 30, 2015

		ntools.eq_(pth_str.best_paths_history, "best_paths_history")


		class TestPathStoreInit(object):

Added Tests for class PathStore in lib/path_store.py #214

Added Tests for class PathStore in lib/path_store.py #214

Conversation

prateshg commented Jun 29, 2015

prateshg commented Jun 29, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kormat commented Jun 30, 2015

prateshg commented Jun 30, 2015

prateshg commented Jun 30, 2015

kormat commented Jun 30, 2015

kormat commented Jun 30, 2015

kormat commented Jun 30, 2015

kormat commented Jun 30, 2015