Code review comment for lp://staging/~wesley-wiedenmeier/curtin/partial-testing

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

I ran checkers and found several of the following (no need to mark them all inline):

tests/storagetest_runner/__init__.py|101 col 16 warning| [unused-variable] Unused variable '_err' [python/pylint]

=> if really unused a _ would be even better.

tests/storagetest_runner/__init__.py|208 col 18 warning| [logging-format-interpolation] Use % formatting in logging functions and pass the % parameters as arguments

=> since log would format for you ...

I know pylint it is noisy and sometimes even disagrees with other checkers, but most of these are also only a search and replace away so probably worth to fix.

I'm not so sure about
tests/storagetests/__init__.py|186 col 16 warning| [unidiomatic-typecheck] Using type() instead of isinstance() for a typecheck. [python/pylint]

tests/storagetests/__init__.py|219 col 24 warning| [redefined-builtin] Redefining built-in 'type' [python/pylint]

I already complained about short names before, this is another example (there are more like mp, fp, e, ...)
tests/storagetests/__init__.py|224 col 13 warning| [invalid-name] Invalid variable name "d" [python/pylint]

A totally different one is:
tests/storagetests/verifiers.py|15 col 13 error| [no-member] Instance of 'BasicTests' has no 'assertIsNotNone' member [python/pylint]
That never is an issue so far, as all inheriting classes of e.g. BasicTests also inherit from class BaseStorageTest(unittest.TestCase). But to make sure the methods are in scope would it hurt letting the verifiers also derive from unittest.TestCase?

One that is probably worth for sure to avoid later issues:
tests/storagetests/__init__.py|82 col 31 warning| [redefined-outer-name] Redefining name 'reporter' from outer scope (line 2) [python/pylint]

« Back to merge proposal