bpo-26544: Add test for platform._comparable_version().#8973
Conversation
vstinner
left a comment
vstinner
left a comment
There was a problem hiding this comment.
See maybe Lib/distutils/tests/test_version.py for ideas of tests. There are tests for strange versions like "1.13++" :-)
|
|
||
| @support.cpython_only | ||
| def test__comparable_version(self): | ||
| from platform import _comparable_version as V |
There was a problem hiding this comment.
What do you think of also adding tests checking directly the result of the function? Compare to a list.
There was a problem hiding this comment.
This would expose too much implementation details. For example the initial versions converted '1.2.3' to [(100, 1), (100, 2), (100, 3)], but now it converts to [100, 1, 100, 2, 100, 3]. I thought also for converting to [1, 2, 3] and using negative numbers for pre-release versions and float('inf') for "pl". If this function be public, I would return a tuple, but a list is enough in this case. Of course magic numbers like 100 are arbitrary.
| @support.cpython_only | ||
| def test__comparable_version(self): | ||
| from platform import _comparable_version as V | ||
| self.assertEqual(V('1.2.3'), V('1.2.3')) |
There was a problem hiding this comment.
If I understood correctly, the function has been added to compare correctly 2.7.9 to 2.7.10 (or a similar issue). Maybe add also a test for that?
Maybe also add a test comparing 2.0 and 2.0.0?
There was a problem hiding this comment.
If I understood correctly, the function has been added to compare correctly 2.7.9 to 2.7.10 (or a similar issue). Maybe add also a test for that?
Next line.
There was a problem hiding this comment.
Maybe also add a test comparing 2.0 and 2.0.0?
Done. Although this is a case for which the result is different from distutils.version.StrictVersion. I don't think this will add problems in practice (in addition to existing problems of using such guessing function).
There was a problem hiding this comment.
Next line.
Oops, I missed it, sorry ;-)
vstinner
left a comment
vstinner
left a comment
There was a problem hiding this comment.
LGTM, thanks for adding tests!
vstinner
left a comment
vstinner
left a comment
There was a problem hiding this comment.
@serhiy-storchaka: can you merge this PR and then update/rewrite your backports to include these new tests?
|
|
||
| @support.cpython_only | ||
| def test__comparable_version(self): | ||
| from platform import _comparable_version as V |
| @support.cpython_only | ||
| def test__comparable_version(self): | ||
| from platform import _comparable_version as V | ||
| self.assertEqual(V('1.2.3'), V('1.2.3')) |
There was a problem hiding this comment.
Next line.
Oops, I missed it, sorry ;-)
4 participants
https://bugs.python.org/issue26544