Make alt-src compatible with Python 3 #26
Conversation
2to3 tool was used to automatically detect code lines which are problematic for Python 3. They were fixed in a way that ensures continued Python 2 compatibility. 'six' package was added as a dependency as it is used in some fixes.
querti
force-pushed
the
make-python3-compatible
branch
from
5ff630b
to
98112e1
Compare
|
Two notes:
|
querti
requested review from
midnightercz,
negillett,
rohanpm,
chandwanitulsi,
JayZ12138,
rajulkumar and
rbikar
tests/test_rpms.py
Outdated
| @@ -658,6 +645,8 @@ def test_push_when_already_pushed(config_file, lookasidedir, default_config, cap | |||
| git_url = default_config['git_push_url'] % {'package':'rcm-repoquery'} | |||
| cmd = ['git', 'tag'] | |||
| out = check_output(cmd, cwd=git_url) | |||
| if not isinstance(out, str) and isinstance(out, bytes): | |||
There was a problem hiding this comment.
Again it should be possible to instead add universal_newlines=True.
There was a problem hiding this comment.
Fixed. I want to point out though that check_output was added in Python 2.7 [1], so even though the project claims to be 2.6 compatible, the test suite is not. I hesitated to fix it because we don't know how many more things have to be fixed before the suite is 2.6 compatible (or 2.4 for that matter)
[1] https://docs.python.org/2/library/subprocess.html#subprocess.check_output
| def test_get_state_with_error_other_than_enoent(tempdir): | ||
| options = MagicMock(koji=None, source=tempfile.mkstemp(dir=tempdir)[1]) | ||
| processor = BaseProcessor(options) | ||
| processor.workdir = tempfile.mkstemp(dir=tempdir)[1] | ||
|
|
||
| # attempting to open state file raises generic IOError | ||
| with patch('__builtin__.open', autospec=True, side_effect=IOError): | ||
| with patch('six.moves.builtins.open', autospec=True, side_effect=IOError): |
There was a problem hiding this comment.
This doesn't make sense to me. My understanding of what happens when you do mock.patch('foo.bar.baz') is that, on foo.bar module, 'baz' will be pointed to a mock object for the duration of the context.
So, this mock would only be used if there was code of the form:
import six.moves.builtins
f = six.moves.builtins.open(...)...and there isn't such code. I don't see any reason that a bare "open()" would be directed to the mock here.
I pulled this change locally, deleted the patch and it didn't change the outcome of the test at all, so this is consistent with this patch not doing anything useful.
Can you please investigate and remove or fix this if it is indeed useless?
There was a problem hiding this comment.
I have investigated the testcase and the reason why the patch is unnecessary is that an actual IOError (not a directory) is raised during the tested classes setup. This is because tempfile is specified as the workdir. So the test incidentally does what it's supposed to, even though it clearly wasn't the intended behaviour. I have fixed the setup and patched 'open' to raise a generic IOError, at least to prevent a user confusion about the intention of the testcase.
Also, it seems that patch doesn't work the way you expected because patching 'six.moves.builtins.open' raises a generic IOError on both Python 2 and 3. Removing the patch now causes it to raise no exceptions.
There was a problem hiding this comment.
Thanks. I had a look into that to figure out where I went wrong and I figured out that mock.patch does work the way I expected, but my conclusion that it wouldn't affect a bare open() was wrong. I learned the reasons why that works:
- 'six.moves.builtins' is the same object as 'builtins', not a wrapper module as I'd thought it might be. So this patch on six.moves.builtins, in python3, is the same as patching on builtins.
>>> id(builtins)
140273681132816
>>> id(six.moves.builtins)
140273681132816
- builtins module is special, apparently anything set there can be looked up implicitly without having to be imported or having to use a qualified name.
>>> setattr(builtins, 'quux', lambda: 123)
>>> quux()
123
alt_src/alt_src.py
Outdated
| @@ -375,7 +381,7 @@ def check_package(self): | |||
| self.logger.debug("Got blacklist: %r", blacklist) | |||
| if blacklist and koji.util.multi_fnmatch(self.package, blacklist): | |||
| # raise FilterError, 'Blacklisted package: %s' % self.package | |||
| print 'Blacklisted package: %s, quitting' % self.package | |||
| print('Blacklisted package: %s, quitting' % self.package) | |||
There was a problem hiding this comment.
Can I ask if there's a reason this is not a part of the log and simply a print statement?
There was a problem hiding this comment.
I have changed the prints to logger messages because there were only two of them. When this code was written, proper review processes were not yet properly implemented, so these kinds of things are quite common in our legacy codebase.
After the automatically detected problems were fixed, the following issues had to be resolved to ensure that the whole test suite passes in Python 3: - in testcase 'test_get_state_with_error_other_than_enoent', '__builtin__' was changed to 'six.moves.builtins' because the built-in library was renamed in Python 3. Also, the testcase itself had been broken because it had raised an IOError due to an incorrect setup (file was assigned as a directory) and not because of the patched 'open' function. It was fixed despite incidentally fulfilling its desired function in order to prevent user confusion. - In Python 3, output for invoked bash commands is a bytes type, which can cause many issues because the code expects to work with a string. Adding the 'universal_newlines' forces the output to be returned as a string. Also, print statements were changed to logger messages.
querti
force-pushed
the
make-python3-compatible
branch
from
98112e1
to
a9098f8
Compare
More info in the commit message.