Add tests #15
Add tests #15
Conversation
| @@ -1 +1,2 @@ | |||
| include *.txt | |||
| include tests.py | |||
There was a problem hiding this comment.
we should not ship tests.py in the package
There was a problem hiding this comment.
Nowadays it's recommended to ship tests too. For example, downstream packagers like Fedora want to run the tests to make sure it's working.
| def assertMappingEqual(self, m1, m2): | ||
| # `assertDictEqual` will make a failing type check when comparing a `dict` | ||
| # with a `frozendict`. Instead, we'll compare the item sets: | ||
| self.assertSetEqual(set(m1.items()), set(m2.items())) |
There was a problem hiding this comment.
there is no assertSetEqual in Python 2.6
There was a problem hiding this comment.
Do we really need assertSetEqual? Because set equality can be tested via == anyways and we can just use assertEqual.
| self.assertSetEqual(set(m1.items()), set(m2.items())) | ||
|
|
||
| def assertRaisesAssignmentError(self, *args, **kwargs): | ||
| self.assertRaisesRegexp( |
There was a problem hiding this comment.
there is no assertRaisesRegexp in Python 2.6
There was a problem hiding this comment.
In 2020 I strongly recommend dropping 2.6 (and consider 2.7).
|
|
||
| class TestFrozenDict(BaseTestCase): | ||
| def test_assignment_fails(self): | ||
| self.assertRaisesAssignmentError(setitem, frozendict(), 'key', 'value') |
There was a problem hiding this comment.
with self.assertRaises(TypeError):
frozendict.key = valuebut assertRaises needs to be backported. I suggest that you copy B2 CLI TestBase.
There was a problem hiding this comment.
Likewise not a problem in 2020 if dropping 2.6.
| def test_assignment_fails(self): | ||
| self.assertRaisesAssignmentError(setitem, frozendict(), 'key', 'value') | ||
|
|
||
| def test_init(self): |
There was a problem hiding this comment.
this test is practically worthless, please remove
| keys = {key for key in fd} | ||
| self.assertSetEqual(keys, {'a', 'b'}) | ||
|
|
||
| def test_len(self): |
There was a problem hiding this comment.
len of empty dict should also be checked
| def test_repr(self): | ||
| fd = frozendict(a=0, b=1) | ||
| r = repr(fd) | ||
| self.assertTrue(r.startswith('<frozendict ')) |
There was a problem hiding this comment.
assertion is too loose, we want to check what is actually in there, not just the header and the footer
|
|
||
| def test_hash(self): | ||
| fd = frozendict(a=0, b=1) | ||
| hash(fd) |
There was a problem hiding this comment.
hashes of two frozendicts which have the same contents, should be identical
| hash(fd) | ||
|
|
||
|
|
||
| class TestFrozenOrderedDict(BaseTestCase): |
There was a problem hiding this comment.
many tests in this class are copies of the tests from class above. Use inheritance and a class field to prevent repetition of code
There was a problem hiding this comment.
Perhaps the FrozenOrderedDict tests should use an assertOrderedMappingEqual function?
There was a problem hiding this comment.
Also, FrozenOrderedDict cannot be tested on Python 2.6 where it is not supported. Maybe raise nose.SkipTest() in setUp?
| self.assertIs(type(copied), type(fd)) | ||
| self.assertMappingEqual(copied, dict(a=0, b=2, c=3)) | ||
|
|
||
| def test_iter(self): |
There was a problem hiding this comment.
there is a 1/16th chance that this test will pass with frozendict. Please increase the test data size to reduce the chance to a more reasonable value
There was a problem hiding this comment.
Hmm, could you explain further?
|
While stdlib's unittest module is quite good, maybe we should use |
|
|
||
| def test_init_with_keys(self): | ||
| fd = frozendict(a=0, b=1) | ||
| self.assertMappingEqual(fd, dict(a=0, b=1)) |
There was a problem hiding this comment.
I'd also be tempted to add something like:
fd1 = frozendict(a=0, b=1)
fd2 = frozendict(b=1, a=0)
self.assertMappingEqual(fd1, fd2)
(and then perhaps the opposite for FrozenOrderedDict)
|
can this be merged? |
@ppolewicz @lurch @eugene-eeo
This project is seeing some activity lately (yay), I think it's time to add some tests.
I incorporated test cases written in an earlier fork, linked by @ppolewicz in #14, but I'm torn between two positions: on one hand, the cases are pretty basic and not much is being actually tested, but on the other hand, most of the real action happens inside
dictandOrderedDict, both provided by the stdlib.I wanted to know your opinions before I merged this in. If you have time, please take a look!