<p>Patch set 4:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p><a href="https://gerrit.osmocom.org/c/pysim/+/24012">View Change</a></p><p>10 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/pysim/+/24012/4/tests/test_files.py">File tests/test_files.py:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/pysim/+/24012/4/tests/test_files.py@22">Patch Set #4, Line 22:</a> <code style="font-family:monospace,monospace">def test_encode_decode(testcase, ef, verbose=False):</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">As I pointed out below, this function should become a method of DecTestCase.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/pysim/+/24012/4/tests/test_files.py@39">Patch Set #4, Line 39:</a> <code style="font-family:monospace,monospace">print</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">It's not a good idea to use print() directly in unit tests. Usually you get a brief summary printed by unittest itself, and now imagine if you have lots of test cases printing some internal stuff... Just use Python's logging framework with DEBUG or INFO level. If something goes wrong, you can always increase the default logging level and see what happens.</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> import logging<br> ...<br> logging.info('foo %s', foo)<br> logging.debug('bar %d', bar)</pre></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/pysim/+/24012/4/tests/test_files.py@49">Patch Set #4, Line 49:</a> <code style="font-family:monospace,monospace"> if not has_testvec:</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This condition looks common for both branches, I would move it below.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/pysim/+/24012/4/tests/test_files.py@57">Patch Set #4, Line 57:</a> <code style="font-family:monospace,monospace"> for testvec_json in ef.__class__._encode_decode_testvector:</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Probably makes sense to wrap each vector into self.subTest too.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/pysim/+/24012/4/tests/test_files.py@60">Patch Set #4, Line 60:</a> <code style="font-family:monospace,monospace">if verbose:</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Again, logging.debug() would do this for you.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/pysim/+/24012/4/tests/test_files.py@71">Patch Set #4, Line 71:</a> <code style="font-family:monospace,monospace"> testcase.assertEqual(testvec_json, decoded_json)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Not sure if it's a good idea to have the test vectors in JSON and match strings. You can execute self.assertEqual on Python objects, and if something does not match, it would give you a very precise diff on fields that differ. After all, imagine JSON library slightly changes the ordering some day...</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/pysim/+/24012/4/tests/test_files.py@82">Patch Set #4, Line 82:</a> <code style="font-family:monospace,monospace">DecTestCase</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Is it only decoding or encoding and decoding together? The name looks confusing.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/pysim/+/24012/4/tests/test_files.py@94">Patch Set #4, Line 94:</a> <code style="font-family:monospace,monospace"> for obj in gc.get_objects():</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Using garbage collector for finding all instances of CardEF is an option, but how can you be sure that you cover _all_ classes? There should be a more elegant solution, like using a meta-class, similar to what unittest framework does: you inherit your test-case class from 'unittest.TestCase' and unittest knows about existence of your inherited class.</p><p style="white-space: pre-wrap; word-wrap: break-word;">There is also built-in property '__subclasses__()', but you would need to expand the returned value recursively.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/pysim/+/24012/4/tests/test_files.py@96">Patch Set #4, Line 96:</a> <code style="font-family:monospace,monospace"> if not file_in_list(test_candidates, obj.name):</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Would be nice to implement the concept of sub-tests here for each file:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> for obj in gc.get_objects():<br> if not isinstance(obj, CardEF):<br> continue<br> with self.subTest(obj.__class__.__name__):<br> test_encode_decode(self, obj)</pre><p style="white-space: pre-wrap; word-wrap: break-word;">This would improve the test execution output.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/pysim/+/24012/4/tests/test_files.py@101">Patch Set #4, Line 101:</a> <code style="font-family:monospace,monospace">test_encode_decode</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">So you have a function that accepts a reference to the test case instance, but for some reason it's not a method of 'DecTestCase' class? This looks very odd to me, why this way?</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/pysim/+/24012">change 24012</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/c/pysim/+/24012"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: pysim </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I02d884547f4982e0b8ed7ef21b8cda75237942e2 </div>
<div style="display:none"> Gerrit-Change-Number: 24012 </div>
<div style="display:none"> Gerrit-PatchSet: 4 </div>
<div style="display:none"> Gerrit-Owner: dexter <pmaier@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: fixeria <vyanitskiy@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Reviewer: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Thu, 06 May 2021 19:44:04 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Gerrit-MessageType: comment </div>