Added bits of refactoring, and a test, removed python27 in tox #48
Conversation
…n27 support in tox
| @@ -8,6 +8,7 @@ | |||
| import os | |||
|
|
|||
| from .syncer import Syncer | |||
| from .options import ArgParser, USER_CFG_FILE, DEFAULT_USER_CFG | |||
|
|
|||
There was a problem hiding this comment.
The reason the options module was only imported within from_cmdline is that it has side effects (creating the options dictionary + arg parser class, etc.), which is not needed at all when calling dirsync from python. That's why I would prefer only importing it from within from_cmdline. If the tests are still needed, they could import ArgParser from the options module.
There was a problem hiding this comment.
personal I do not like import in functions. I like to have all the import at the top of the file where I would expect them to find. When I look at the options module, I think having the import of the options module only for cmd_line scope, would be a premature optimization.
There was a problem hiding this comment.
I do not like imports in functions either and endeavour to put all imports at the top, but in a few cases like this one they make sense. Imports in python are pretty expensive (especially as the options module imports other modules) so if it is easy to avoid importing modules we know we won't need in some well-defined use cases, we do it.
A change that is trivial to implement and understand and can save 10s of ms each time a module is imported is not 'premature optimisation' (a term that has been debased), it's just common sense!
If we really wanted to avoid imports in functions, you could have create a separate from_cmdline module that would import the bits it needs from options, but as from_cmdline and run essentially do the same thing, it makes a lot of sense to keep them in the same module.
Would you mind reverting that change please? Thanks in advance.
tests/test_run_from_cmdline.py
Outdated
| with patch.object(run, "sync"), patch.object(run, "ArgParser"): | ||
| run.from_cmdline() | ||
|
|
||
| assert mock_file.called |
There was a problem hiding this comment.
wouldn't we want to check a few more things? i.e. with what arguments open() has been called?
| arg_parser_mock.side_effect = Exception() | ||
| with patch.object(run, "sys") as sys_mock: | ||
| run.from_cmdline() | ||
| sys_mock.exit.assert_called_with(2) |
There was a problem hiding this comment.
Is that test checking that the script exits when the arg parser raises an exception? If yes I'm not sure I understand the point of running it ... but maybe I'm missing something?
There was a problem hiding this comment.
well, I see this part of the test coverage, without this test the exception context of the try sync is not tested. My aim is always to have 100% coverage on my code, as experience has shown that the bugs usually hide in the few percent not covered.
There was a problem hiding this comment.
OK, I made that remark believing from_cmdline was already tested (see below) and that there would be quite a lot of more relevant not-covered parts of dirsync that would deserve attention first. But you definitely have a point, and the fact that from_cmdline was not tested at all before makes it a lot more relevant.
I indeed realised that the cmdline test module where your tests could (and probably should) be located only called dirsync.run.run. I think the CmdLineTests.dirsync method should be updated there to call from_cmdline instead of dirsync.run, and the arguments should be fed into sys.args instead of being passed.
In this module you'll see that the decorator syntax (@patch(...)) is preferred to the context manager syntax (with patch(...)) for all patches required in the test case. This gets rid of quite a few indents and makes the test a lot more readable.
If you are happy with that, it would be great to:
- modify
tests.cmline.CmdLineTests.dirsyncso that it callsfrom_cmdline - move your tests in the
tests.cmdlinemodule and make them inherit fromCmdLineTests - use
@patchon the test method to avoid the context managers in your tests
Sorry for not having had a closer look in my previous review and thanks again for the contributions.
| @@ -1,5 +1,5 @@ | |||
| [tox] | |||
| envlist = py27, py36, py37, py38, py39 | |||
| envlist = py36, py37, py38, py39 | |||
There was a problem hiding this comment.
Yes, I agree 2.7 should be dropped (there is a bit of clean-up to do to fully remove six and python 2 specific code). I'll drop 3.6 too, and add 3.10 (and soon 3.11)
| @@ -8,6 +8,7 @@ | |||
| import os | |||
|
|
|||
| from .syncer import Syncer | |||
| from .options import ArgParser, USER_CFG_FILE, DEFAULT_USER_CFG | |||
|
|
|||
There was a problem hiding this comment.
I do not like imports in functions either and endeavour to put all imports at the top, but in a few cases like this one they make sense. Imports in python are pretty expensive (especially as the options module imports other modules) so if it is easy to avoid importing modules we know we won't need in some well-defined use cases, we do it.
A change that is trivial to implement and understand and can save 10s of ms each time a module is imported is not 'premature optimisation' (a term that has been debased), it's just common sense!
If we really wanted to avoid imports in functions, you could have create a separate from_cmdline module that would import the bits it needs from options, but as from_cmdline and run essentially do the same thing, it makes a lot of sense to keep them in the same module.
Would you mind reverting that change please? Thanks in advance.
| arg_parser_mock.side_effect = Exception() | ||
| with patch.object(run, "sys") as sys_mock: | ||
| run.from_cmdline() | ||
| sys_mock.exit.assert_called_with(2) |
There was a problem hiding this comment.
OK, I made that remark believing from_cmdline was already tested (see below) and that there would be quite a lot of more relevant not-covered parts of dirsync that would deserve attention first. But you definitely have a point, and the fact that from_cmdline was not tested at all before makes it a lot more relevant.
I indeed realised that the cmdline test module where your tests could (and probably should) be located only called dirsync.run.run. I think the CmdLineTests.dirsync method should be updated there to call from_cmdline instead of dirsync.run, and the arguments should be fed into sys.args instead of being passed.
In this module you'll see that the decorator syntax (@patch(...)) is preferred to the context manager syntax (with patch(...)) for all patches required in the test case. This gets rid of quite a few indents and makes the test a lot more readable.
If you are happy with that, it would be great to:
- modify
tests.cmline.CmdLineTests.dirsyncso that it callsfrom_cmdline - move your tests in the
tests.cmdlinemodule and make them inherit fromCmdLineTests - use
@patchon the test method to avoid the context managers in your tests
Sorry for not having had a closer look in my previous review and thanks again for the contributions.
This module help me a lot during my work. So I want to return a (small) favor.
I did my best to have python27 also running, but windows was not cooperating. As python 2.7 is EOL since two years now, I think is save not to support it any more.
When appreciated I can refactor some more, and increate the test coverage. In future PRs.