bpo-24263: Fix unittest to discover tests named with non-ascii characters#13149
bpo-24263: Fix unittest to discover tests named with non-ascii characters#13149abadger wants to merge 3 commits intopython:mainfrom
Conversation
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. |
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().
There was a problem hiding this comment.
I've added this as a private function in loader: _is_valid_module_filename(). The code that makes use of the function makes use of it to tell if a filename is a valid python module name. A different code path operates on directory names. I think I put the amount of logic into the function that you were thinking (the equivalent of what the regex replaced). Let me know if you were thinking of something with a larger scope.
3ad3161 to
9273219
Compare
* The unittest 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 * Add a changelog entry for the unittest test discovery fix * Remove python2-straddling code * Merge test_find_tests_with_unicode with test_find_tests as they are really just variants on the data provided. * Simplify a few sections of the code as suggested by ezio melotti
9273219 to
688e2ef
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. |
|
@ezio-melotti please review the changes. I found the issue and PR as part of EuroPython 24 sprints. |
|
This PR is stale because it has been open for 30 days with no activity. |
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