bpo-24263: Fix unittest to discover tests named with non-ascii characters #13149
Conversation
The unitteset test discovery was incorrectly looking for tests in directories which had non-identifiers in the directory name. This change makes the discovery only enter directories which can be valid python identifiers
abadger
force-pushed
the
rebase-unittest-unicode-pattern
branch
from
cf97858
to
86faeec
Compare
Lib/unittest/test/test_discovery.py
Outdated
| path_lists = [['test2.py', 'test1.py', 'not_a_test.py', 'test_dir', | ||
| 'test.foo', 'test-not-a-module.py', 'another_dir'], | ||
| path_lists = [ # Valid module and package names | ||
| ['test2.py', 'test1.py', 'not_a_test.py', 'test_dir', |
There was a problem hiding this comment.
I would move the [ before the comment, otherwise it seems like the comment applies to the whole list.
Lib/unittest/test/test_discovery.py
Outdated
| # of a package directory | ||
| ['測試4.py', '測試3.py', ]] | ||
| os.listdir = lambda path: path_lists.pop(0) | ||
| self.addCleanup(restore_listdir) |
There was a problem hiding this comment.
Instead of defining all the restore_* functions, I would just do something like self.addCleanup(setattr, os, 'listdir', original_listdir).
Another option is using mock.patch, but maybe here it's overkill.
There was a problem hiding this comment.
So, for the remainder of these comments.... It appears that this test_find_tests_with_unicode() function is a copy of the pre-existing test, test_find_tests(), adjusted to use new data.
If you'd like me to make these changes, I can, but I think I'd pair it with merging the two tests.
Lib/unittest/test/test_discovery.py
Outdated
|
|
||
| def isfile(path): | ||
| # '另外的_資料夾' is not a package and so shouldn't be recursed into | ||
| return not path.endswith('資料夾') and not '另外的_資料夾' in path |
There was a problem hiding this comment.
return not path.endswith('資料夾') and '另外的_資料夾' not in path
Lib/unittest/test/test_discovery.py
Outdated
|
|
||
| def isdir(path): | ||
| return path.endswith('資料夾') | ||
| os.path.isdir = isdir |
There was a problem hiding this comment.
Any reason why here you defined an isdir function instead of using a lambda?
(FWIW I think for the sake of brevity lambdas are fine in this context.)
Lib/unittest/test/test_discovery.py
Outdated
| # execution order. | ||
| expected = [[name + ' module tests'] for name in | ||
| ('測試1', '測試2', '測試_資料夾')] | ||
| expected.extend([[('測試_資料夾.%s' % name) + ' module tests'] for name in |
There was a problem hiding this comment.
expected.extend([['測試_資料夾.%s module tests' % name] for name in
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
|
@rbtcollins, it seems like the people presently interested in this PR are leaning towards removing the Python2 compatible code so I'm going to go ahead and remove that. |
|
@ezio-melotti Updated. I believe with all of the changes that you and @vstinner asked for. |
abadger
force-pushed
the
rebase-unittest-unicode-pattern
branch
from
d870a5e
to
883876f
Compare
* Merge these two discovery tests as they really are just variants on the data provided rather than the code of the test. * Simplify a few sections of the code as suggested by ezio melotti * Fix some formatting issues
abadger
force-pushed
the
rebase-unittest-unicode-pattern
branch
from
883876f
to
b4808be
Compare
|
I have made the requested changes; please review again |
|
Thanks for making the requested changes! @ezio-melotti: please review the changes made to this pull request. |
| # what about .pyc (etc) | ||
| # we would need to avoid loading the same tests multiple times | ||
| # from '.py', *and* '.pyc' | ||
| VALID_MODULE_NAME = re.compile(r'[_a-z]\w*\.py$', re.IGNORECASE) |
There was a problem hiding this comment.
I think it would be better to keep this global and update it to use r'[^\W\d]\w*\.py$', but as @serhiy-storchaka pointed out in #68451 (comment) this doesn't match all identifiers.
Perhaps a is_valid_module_name() global function could be added instead -- this will still provide a way to control what is valid, instead of hardcoding the check in _find_test_path().
This is a continuation of #1338 from @louisom which is itself based on a patch in the bpo issue tracker from
sih4sing5hong5.In addition to rebasing louisom's work to current master, I've done some work to address comments on the earlier code:
I've added some comments to explain both the test data and why the AttributeError catching is present (it is to enable backporting to the externally maintained unittest package) which is something that @serhiy-storchaka was wondering about in an earlier review: https://bugs.python.org/review/24263/diff/15147/Lib/unittest/loader.py#newcode425
@rbtcollins reviewed the earlier attempt and said:
I've addressed that by adding a test using isidentifier() in the isdirectory section of the conditional: https://github.com/python/cpython/pull/13149/files#diff-c1c31e722f730423a5e6dcc3eb6eef67R471
and adding directory names which are not valid identifiers (but have
__init__.pyinside of them) to the test cases.https://bugs.python.org/issue24263