From ec22a674399162a658209a7f5ffffc79c3e86fa3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mauricio=20V=C3=A1squez?= Date: Wed, 2 Oct 2019 16:47:38 -0500 Subject: [PATCH 1/4] fix bug in BoundedList for python 3.4 and add tests collections.deque.copy() was introduced in python 3.5, this commit changes that by the deque constructor and adds some tests to BoundedList and BoundedDict to avoid similar problems in the future. --- .../src/opentelemetry/sdk/util.py | 4 +- opentelemetry-sdk/tests/test_util.py | 162 ++++++++++++++++++ 2 files changed, 164 insertions(+), 2 deletions(-) create mode 100644 opentelemetry-sdk/tests/test_util.py diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/util.py b/opentelemetry-sdk/src/opentelemetry/sdk/util.py index ede52e8307..829e7afa25 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/util.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/util.py @@ -62,7 +62,7 @@ def __len__(self): def __iter__(self): with self._lock: - return iter(self._dq.copy()) + return iter(deque(self._dq)) def append(self, item): with self._lock: @@ -89,7 +89,7 @@ def from_seq(cls, maxlen, seq): class BoundedDict(MutableMapping): - """A dict with a fixed max capacity.""" + """An ordered dict with a fixed max capacity.""" def __init__(self, maxlen): if not isinstance(maxlen, int): diff --git a/opentelemetry-sdk/tests/test_util.py b/opentelemetry-sdk/tests/test_util.py new file mode 100644 index 0000000000..11bc18659d --- /dev/null +++ b/opentelemetry-sdk/tests/test_util.py @@ -0,0 +1,162 @@ +# Copyright 2019, OpenTelemetry Authors +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import collections +import unittest + +from opentelemetry.sdk.util import BoundedDict, BoundedList + + +class TestBoundedList(unittest.TestCase): + base = [52, 36, 53, 29, 54, 99, 56, 48, 22, 35, 21, 65, 10, 95, 42, 60] + + def test_negative_maxlen(self): + with self.assertRaises(ValueError): + BoundedList(-1) + + def test_from_seq(self): + list_len = len(TestBoundedList.base) + blist = BoundedList.from_seq(list_len, TestBoundedList.base) + + self.assertEqual(len(blist), list_len) + + for idx in range(list_len): + self.assertEqual(blist[idx], TestBoundedList.base[idx]) + + # sequence too big + with self.assertRaises(ValueError): + BoundedList.from_seq(list_len / 2, TestBoundedList.base) + + def test_append_no_drop(self): + # create empty list + list_len = len(TestBoundedList.base) + blist = BoundedList(list_len) + self.assertEqual(len(blist), 0) + + # fill list + for idx in TestBoundedList.base: + blist.append(idx) + + self.assertEqual(len(blist), list_len) + self.assertEqual(blist.dropped, 0) + + for idx in range(list_len): + self.assertEqual(blist[idx], TestBoundedList.base[idx]) + + # test __iter__ in BoundedList + idx = 0 + for val in blist: + self.assertEqual(val, TestBoundedList.base[idx]) + idx += 1 + + def test_append_drop(self): + list_len = len(TestBoundedList.base) + # create full BoundedList + blist = BoundedList.from_seq(list_len, TestBoundedList.base) + + # try to append more items + for idx in range(8): + # should drop the element without raising exceptions + blist.append(idx) + + self.assertEqual(len(blist), list_len) + self.assertEqual(blist.dropped, 8) + + def test_extend_no_drop(self): + # create empty list + list_len = len(TestBoundedList.base) + blist = BoundedList(list_len) + self.assertEqual(len(blist), 0) + + # fill list + blist.extend(TestBoundedList.base) + + self.assertEqual(len(blist), list_len) + self.assertEqual(blist.dropped, 0) + + for idx in range(list_len): + self.assertEqual(blist[idx], TestBoundedList.base[idx]) + + # test __iter__ in BoundedList + idx = 0 + for val in blist: + self.assertEqual(val, TestBoundedList.base[idx]) + idx += 1 + + def test_extend_drop(self): + list_len = len(TestBoundedList.base) + # create full BoundedList + blist = BoundedList.from_seq(list_len, TestBoundedList.base) + other_list = [13, 37, 51, 91] + + # try to extend with more elements + blist.extend(other_list) + + self.assertEqual(len(blist), list_len) + self.assertEqual(blist.dropped, len(other_list)) + + +class TestBoundedDict(unittest.TestCase): + base = collections.OrderedDict( + [ + ("name", "Firulais"), + ("age", 7), + ("weight", 13), + ("vaccinated", True), + ] + ) + + def test_negative_maxlen(self): + with self.assertRaises(ValueError): + BoundedDict(-1) + + def test_from_map(self): + dic_len = len(TestBoundedDict.base) + bdict = BoundedDict.from_map(dic_len, TestBoundedDict.base) + + self.assertEqual(len(bdict), dic_len) + + for key in TestBoundedDict.base: + self.assertEqual(bdict[key], TestBoundedDict.base[key]) + + # map too big + with self.assertRaises(ValueError): + BoundedDict.from_map(dic_len / 2, TestBoundedDict.base) + + def test_bounded_dict(self): + # create empty dict + dic_len = len(TestBoundedDict.base) + bdict = BoundedDict(dic_len) + self.assertEqual(len(bdict), 0) + + # fill list + for key in TestBoundedDict.base: + bdict[key] = TestBoundedDict.base[key] + + self.assertEqual(len(bdict), dic_len) + self.assertEqual(bdict.dropped, 0) + + for key in TestBoundedDict.base: + self.assertEqual(bdict[key], TestBoundedDict.base[key]) + + # test __iter__ in BoundedList + for key in bdict: + self.assertEqual(bdict[key], TestBoundedDict.base[key]) + + # try to append more elements + for key in TestBoundedDict.base: + bdict["new-key" + key] = TestBoundedDict.base[key] + + self.assertEqual(len(bdict), dic_len) + self.assertEqual(bdict.dropped, dic_len) From 897ba7c4bc93086d817b1fc52108fc342b84b20d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mauricio=20V=C3=A1squez?= Date: Thu, 3 Oct 2019 12:21:06 -0500 Subject: [PATCH 2/4] handle feedback - assert that changing the base list/dict does not change the BoundedList/BoundedDict - some variables renaming - test that dropped elements are the old ones --- opentelemetry-sdk/tests/test_util.py | 70 +++++++++++++++++++++++----- 1 file changed, 59 insertions(+), 11 deletions(-) diff --git a/opentelemetry-sdk/tests/test_util.py b/opentelemetry-sdk/tests/test_util.py index 11bc18659d..cf3e3c2540 100644 --- a/opentelemetry-sdk/tests/test_util.py +++ b/opentelemetry-sdk/tests/test_util.py @@ -21,19 +21,40 @@ class TestBoundedList(unittest.TestCase): base = [52, 36, 53, 29, 54, 99, 56, 48, 22, 35, 21, 65, 10, 95, 42, 60] - def test_negative_maxlen(self): + def test_raises(self): with self.assertRaises(ValueError): BoundedList(-1) + blist = BoundedList(4) + blist.append(37) + blist.append(13) + + with self.assertRaises(IndexError): + blist[2] + + with self.assertRaises(IndexError): + blist[4] + + with self.assertRaises(IndexError): + blist[-3] + def test_from_seq(self): list_len = len(TestBoundedList.base) - blist = BoundedList.from_seq(list_len, TestBoundedList.base) + base_copy = list(TestBoundedList.base) + blist = BoundedList.from_seq(list_len, base_copy) self.assertEqual(len(blist), list_len) + # modify base_copy and test that blist is not changed + for idx in range(list_len): + base_copy[idx] = idx * base_copy[idx] + for idx in range(list_len): self.assertEqual(blist[idx], TestBoundedList.base[idx]) + # test that iter yields the correct number of elements + self.assertEqual(len(tuple(blist)), list_len) + # sequence too big with self.assertRaises(ValueError): BoundedList.from_seq(list_len / 2, TestBoundedList.base) @@ -45,8 +66,8 @@ def test_append_no_drop(self): self.assertEqual(len(blist), 0) # fill list - for idx in TestBoundedList.base: - blist.append(idx) + for item in TestBoundedList.base: + blist.append(item) self.assertEqual(len(blist), list_len) self.assertEqual(blist.dropped, 0) @@ -66,12 +87,16 @@ def test_append_drop(self): blist = BoundedList.from_seq(list_len, TestBoundedList.base) # try to append more items - for idx in range(8): + for idx in range(list_len): # should drop the element without raising exceptions - blist.append(idx) + blist.append(2 * TestBoundedList.base[idx]) self.assertEqual(len(blist), list_len) - self.assertEqual(blist.dropped, 8) + self.assertEqual(blist.dropped, list_len) + + # test that new elements are in the list + for idx in range(list_len): + self.assertEqual(blist[idx], 2 * TestBoundedList.base[idx]) def test_extend_no_drop(self): # create empty list @@ -123,13 +148,21 @@ def test_negative_maxlen(self): def test_from_map(self): dic_len = len(TestBoundedDict.base) - bdict = BoundedDict.from_map(dic_len, TestBoundedDict.base) + base_copy = collections.OrderedDict(TestBoundedDict.base) + bdict = BoundedDict.from_map(dic_len, base_copy) self.assertEqual(len(bdict), dic_len) + # modify base_copy and test that bdict is not changed + base_copy["name"] = "Bruno" + base_copy["age"] = 3 + for key in TestBoundedDict.base: self.assertEqual(bdict[key], TestBoundedDict.base[key]) + # test that iter yields the correct number of elements + self.assertEqual(len(tuple(bdict)), dic_len) + # map too big with self.assertRaises(ValueError): BoundedDict.from_map(dic_len / 2, TestBoundedDict.base) @@ -140,7 +173,7 @@ def test_bounded_dict(self): bdict = BoundedDict(dic_len) self.assertEqual(len(bdict), 0) - # fill list + # fill dict for key in TestBoundedDict.base: bdict[key] = TestBoundedDict.base[key] @@ -150,13 +183,28 @@ def test_bounded_dict(self): for key in TestBoundedDict.base: self.assertEqual(bdict[key], TestBoundedDict.base[key]) - # test __iter__ in BoundedList + # test __iter__ in BoundedDict for key in bdict: self.assertEqual(bdict[key], TestBoundedDict.base[key]) + # updating an existing element should not drop + bdict["name"] = "Bruno" + self.assertEqual(bdict.dropped, 0) + # try to append more elements for key in TestBoundedDict.base: - bdict["new-key" + key] = TestBoundedDict.base[key] + bdict["new-" + key] = TestBoundedDict.base[key] self.assertEqual(len(bdict), dic_len) self.assertEqual(bdict.dropped, dic_len) + + # test that elements in the dict are the new ones + for key in TestBoundedDict.base: + self.assertEqual(bdict["new-" + key], TestBoundedDict.base[key]) + + # delete an element + del bdict["new-name"] + self.assertEqual(len(bdict), dic_len - 1) + + with self.assertRaises(KeyError): + bdict["new-name"] \ No newline at end of file From 038c21e5ec9914f987f09b6103540a78b0e3f92e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mauricio=20V=C3=A1squez?= Date: Thu, 3 Oct 2019 12:32:42 -0500 Subject: [PATCH 3/4] make lint happy --- opentelemetry-sdk/tests/test_util.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/opentelemetry-sdk/tests/test_util.py b/opentelemetry-sdk/tests/test_util.py index cf3e3c2540..c092d9ed71 100644 --- a/opentelemetry-sdk/tests/test_util.py +++ b/opentelemetry-sdk/tests/test_util.py @@ -30,13 +30,13 @@ def test_raises(self): blist.append(13) with self.assertRaises(IndexError): - blist[2] + _ = blist[2] with self.assertRaises(IndexError): - blist[4] + _ = blist[4] with self.assertRaises(IndexError): - blist[-3] + _ = blist[-3] def test_from_seq(self): list_len = len(TestBoundedList.base) @@ -207,4 +207,4 @@ def test_bounded_dict(self): self.assertEqual(len(bdict), dic_len - 1) with self.assertRaises(KeyError): - bdict["new-name"] \ No newline at end of file + _ = bdict["new-name"] From 4579754bec7715359814f40138c96fcc3cb209d7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mauricio=20V=C3=A1squez?= Date: Fri, 4 Oct 2019 13:44:29 -0500 Subject: [PATCH 4/4] handle review - improve docstrings of testing cases - use self.base instead of .base - improve docstrings of BoundedList and BoundedDict classes - use enumerate() where possible --- .../src/opentelemetry/sdk/util.py | 12 ++- opentelemetry-sdk/tests/test_util.py | 83 ++++++++++--------- 2 files changed, 53 insertions(+), 42 deletions(-) diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/util.py b/opentelemetry-sdk/src/opentelemetry/sdk/util.py index 829e7afa25..c2df2b3f4e 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/util.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/util.py @@ -42,7 +42,11 @@ def ns_to_iso_str(nanoseconds): class BoundedList(Sequence): - """An append only list with a fixed max size.""" + """An append only list with a fixed max size. + + Calls to `append` and `extend` will drop the oldest elements if there is + not enough room. + """ def __init__(self, maxlen): self.dropped = 0 @@ -89,7 +93,11 @@ def from_seq(cls, maxlen, seq): class BoundedDict(MutableMapping): - """An ordered dict with a fixed max capacity.""" + """An ordered dict with a fixed max capacity. + + Oldest elements are dropped when the dict is full and a new element is + added. + """ def __init__(self, maxlen): if not isinstance(maxlen, int): diff --git a/opentelemetry-sdk/tests/test_util.py b/opentelemetry-sdk/tests/test_util.py index c092d9ed71..ead310bd8d 100644 --- a/opentelemetry-sdk/tests/test_util.py +++ b/opentelemetry-sdk/tests/test_util.py @@ -22,6 +22,11 @@ class TestBoundedList(unittest.TestCase): base = [52, 36, 53, 29, 54, 99, 56, 48, 22, 35, 21, 65, 10, 95, 42, 60] def test_raises(self): + """Test corner cases + + - negative list size + - access out of range indexes + """ with self.assertRaises(ValueError): BoundedList(-1) @@ -39,8 +44,8 @@ def test_raises(self): _ = blist[-3] def test_from_seq(self): - list_len = len(TestBoundedList.base) - base_copy = list(TestBoundedList.base) + list_len = len(self.base) + base_copy = list(self.base) blist = BoundedList.from_seq(list_len, base_copy) self.assertEqual(len(blist), list_len) @@ -50,79 +55,77 @@ def test_from_seq(self): base_copy[idx] = idx * base_copy[idx] for idx in range(list_len): - self.assertEqual(blist[idx], TestBoundedList.base[idx]) + self.assertEqual(blist[idx], self.base[idx]) # test that iter yields the correct number of elements self.assertEqual(len(tuple(blist)), list_len) # sequence too big with self.assertRaises(ValueError): - BoundedList.from_seq(list_len / 2, TestBoundedList.base) + BoundedList.from_seq(list_len / 2, self.base) def test_append_no_drop(self): + """Append max capacity elements to the list without dropping elements.""" # create empty list - list_len = len(TestBoundedList.base) + list_len = len(self.base) blist = BoundedList(list_len) self.assertEqual(len(blist), 0) # fill list - for item in TestBoundedList.base: + for item in self.base: blist.append(item) self.assertEqual(len(blist), list_len) self.assertEqual(blist.dropped, 0) for idx in range(list_len): - self.assertEqual(blist[idx], TestBoundedList.base[idx]) + self.assertEqual(blist[idx], self.base[idx]) # test __iter__ in BoundedList - idx = 0 - for val in blist: - self.assertEqual(val, TestBoundedList.base[idx]) - idx += 1 + for idx, val in enumerate(blist): + self.assertEqual(val, self.base[idx]) def test_append_drop(self): - list_len = len(TestBoundedList.base) + """Append more than max capacity elements and test that oldest ones are dropped.""" + list_len = len(self.base) # create full BoundedList - blist = BoundedList.from_seq(list_len, TestBoundedList.base) + blist = BoundedList.from_seq(list_len, self.base) # try to append more items - for idx in range(list_len): + for val in self.base: # should drop the element without raising exceptions - blist.append(2 * TestBoundedList.base[idx]) + blist.append(2 * val) self.assertEqual(len(blist), list_len) self.assertEqual(blist.dropped, list_len) # test that new elements are in the list for idx in range(list_len): - self.assertEqual(blist[idx], 2 * TestBoundedList.base[idx]) + self.assertEqual(blist[idx], 2 * self.base[idx]) def test_extend_no_drop(self): # create empty list - list_len = len(TestBoundedList.base) + list_len = len(self.base) blist = BoundedList(list_len) self.assertEqual(len(blist), 0) # fill list - blist.extend(TestBoundedList.base) + blist.extend(self.base) self.assertEqual(len(blist), list_len) self.assertEqual(blist.dropped, 0) for idx in range(list_len): - self.assertEqual(blist[idx], TestBoundedList.base[idx]) + self.assertEqual(blist[idx], self.base[idx]) # test __iter__ in BoundedList - idx = 0 - for val in blist: - self.assertEqual(val, TestBoundedList.base[idx]) - idx += 1 + for idx, val in enumerate(blist): + self.assertEqual(val, self.base[idx]) def test_extend_drop(self): - list_len = len(TestBoundedList.base) + list_len = len(self.base) # create full BoundedList - blist = BoundedList.from_seq(list_len, TestBoundedList.base) + blist = BoundedList.from_seq(list_len, self.base) other_list = [13, 37, 51, 91] # try to extend with more elements @@ -147,8 +150,8 @@ def test_negative_maxlen(self): BoundedDict(-1) def test_from_map(self): - dic_len = len(TestBoundedDict.base) - base_copy = collections.OrderedDict(TestBoundedDict.base) + dic_len = len(self.base) + base_copy = collections.OrderedDict(self.base) bdict = BoundedDict.from_map(dic_len, base_copy) self.assertEqual(len(bdict), dic_len) @@ -157,50 +160,50 @@ def test_from_map(self): base_copy["name"] = "Bruno" base_copy["age"] = 3 - for key in TestBoundedDict.base: - self.assertEqual(bdict[key], TestBoundedDict.base[key]) + for key in self.base: + self.assertEqual(bdict[key], self.base[key]) # test that iter yields the correct number of elements self.assertEqual(len(tuple(bdict)), dic_len) # map too big with self.assertRaises(ValueError): - BoundedDict.from_map(dic_len / 2, TestBoundedDict.base) + BoundedDict.from_map(dic_len / 2, self.base) def test_bounded_dict(self): # create empty dict - dic_len = len(TestBoundedDict.base) + dic_len = len(self.base) bdict = BoundedDict(dic_len) self.assertEqual(len(bdict), 0) # fill dict - for key in TestBoundedDict.base: - bdict[key] = TestBoundedDict.base[key] + for key in self.base: + bdict[key] = self.base[key] self.assertEqual(len(bdict), dic_len) self.assertEqual(bdict.dropped, 0) - for key in TestBoundedDict.base: - self.assertEqual(bdict[key], TestBoundedDict.base[key]) + for key in self.base: + self.assertEqual(bdict[key], self.base[key]) # test __iter__ in BoundedDict for key in bdict: - self.assertEqual(bdict[key], TestBoundedDict.base[key]) + self.assertEqual(bdict[key], self.base[key]) # updating an existing element should not drop bdict["name"] = "Bruno" self.assertEqual(bdict.dropped, 0) # try to append more elements - for key in TestBoundedDict.base: - bdict["new-" + key] = TestBoundedDict.base[key] + for key in self.base: + bdict["new-" + key] = self.base[key] self.assertEqual(len(bdict), dic_len) self.assertEqual(bdict.dropped, dic_len) # test that elements in the dict are the new ones - for key in TestBoundedDict.base: - self.assertEqual(bdict["new-" + key], TestBoundedDict.base[key]) + for key in self.base: + self.assertEqual(bdict["new-" + key], self.base[key]) # delete an element del bdict["new-name"]