Change in pysim[master]: filesystem: add unit tests for encoder/decoder methods

This is merely a historical archive of years 2008-2021, before the migration to mailman3.

A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.

fixeria gerrit-no-reply at lists.osmocom.org
Thu May 6 19:44:04 UTC 2021


fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/pysim/+/24012 )

Change subject: filesystem: add unit tests for encoder/decoder methods
......................................................................


Patch Set 4: Code-Review-1

(10 comments)

https://gerrit.osmocom.org/c/pysim/+/24012/4/tests/test_files.py 
File tests/test_files.py:

https://gerrit.osmocom.org/c/pysim/+/24012/4/tests/test_files.py@22 
PS4, Line 22: def test_encode_decode(testcase, ef, verbose=False):
As I pointed out below, this function should become a method of DecTestCase.


https://gerrit.osmocom.org/c/pysim/+/24012/4/tests/test_files.py@39 
PS4, Line 39: print
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.

  import logging
  ...
  logging.info('foo %s', foo)
  logging.debug('bar %d', bar)


https://gerrit.osmocom.org/c/pysim/+/24012/4/tests/test_files.py@49 
PS4, Line 49: 		if not has_testvec:
This condition looks common for both branches, I would move it below.


https://gerrit.osmocom.org/c/pysim/+/24012/4/tests/test_files.py@57 
PS4, Line 57: 	for testvec_json in ef.__class__._encode_decode_testvector:
Probably makes sense to wrap each vector into self.subTest too.


https://gerrit.osmocom.org/c/pysim/+/24012/4/tests/test_files.py@60 
PS4, Line 60: if verbose:
Again, logging.debug() would do this for you.


https://gerrit.osmocom.org/c/pysim/+/24012/4/tests/test_files.py@71 
PS4, Line 71: 		testcase.assertEqual(testvec_json, decoded_json)
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...


https://gerrit.osmocom.org/c/pysim/+/24012/4/tests/test_files.py@82 
PS4, Line 82: DecTestCase
Is it only decoding or encoding and decoding together? The name looks confusing.


https://gerrit.osmocom.org/c/pysim/+/24012/4/tests/test_files.py@94 
PS4, Line 94: 		for obj in gc.get_objects():
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.

There is also built-in property '__subclasses__()', but you would need to expand the returned value recursively.


https://gerrit.osmocom.org/c/pysim/+/24012/4/tests/test_files.py@96 
PS4, Line 96: 				if not file_in_list(test_candidates, obj.name):
Would be nice to implement the concept of sub-tests here for each file:

  for obj in gc.get_objects():
    if not isinstance(obj, CardEF):
      continue
    with self.subTest(obj.__class__.__name__):
      test_encode_decode(self, obj)

This would improve the test execution output.


https://gerrit.osmocom.org/c/pysim/+/24012/4/tests/test_files.py@101 
PS4, Line 101: test_encode_decode
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?



-- 
To view, visit https://gerrit.osmocom.org/c/pysim/+/24012
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: pysim
Gerrit-Branch: master
Gerrit-Change-Id: I02d884547f4982e0b8ed7ef21b8cda75237942e2
Gerrit-Change-Number: 24012
Gerrit-PatchSet: 4
Gerrit-Owner: dexter <pmaier at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy at sysmocom.de>
Gerrit-Reviewer: laforge <laforge at osmocom.org>
Gerrit-Reviewer: pespin <pespin at sysmocom.de>
Gerrit-Comment-Date: Thu, 06 May 2021 19:44:04 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20210506/00eb56b1/attachment.htm>


More information about the gerrit-log mailing list