classification
Title: Merge all (non-syntactic) import-related tests into test_importlib
Type: enhancement Stage: resolved
Components: Tests Versions: Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Arfrever, aeros, brett.cannon, eric.snow, gforcada, miss-islington
Priority: low Keywords: easy, patch

Created on 2013-11-22 16:04 by brett.cannon, last changed 2019-07-15 18:52 by brett.cannon. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 14303 merged aeros, 2019-06-22 02:45
PR 14466 merged aeros, 2019-06-29 15:56
PR 14537 closed aeros, 2019-07-01 23:27
PR 14582 merged aeros, 2019-07-04 07:38
PR 14642 closed aeros, 2019-07-08 06:57
PR 14655 merged aeros, 2019-07-09 04:23
Messages (20)
msg203792 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2013-11-22 16:04
E.g. test_namespace_pkgs should be under test_importlib and so should test_namespace_pkgs. test_import can conceivably stay out if it's updated to only contain syntactic tests for the import statement.

This is so that it's easier to run import-related tests when making changes to importlib (otherwise one has to run the whole test suite or memorize the name of every test suite related to import/importlib).
msg203833 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2013-11-22 18:52
+1
msg345783 - (view) Author: Gil Forcada Codinachs (gforcada) * Date: 2019-06-16 21:54
At least test_namespace_pkgs is already moved, see https://bugs.python.org/issue21097

Should then this issue be closed?

I see that there are a few other tests that have import on its name and are outside either test_import or test_importlib:

- test_pkgimport.py
- test_threaded_import.py
- test_zipimport.py
- test_zipimport_support.py
- threaded_import_hangers.py

Should those be actually moved to test_importlib?
msg345910 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2019-06-17 21:17
Anything zipimport-related should stay separate as zipimport is not a part of importlib. The other three will have to be looked at to see what exactly they are testing to know whether they relate to importlib and the implementation of import or not (my guess is yes).
msg346184 - (view) Author: Kyle Stanley (aeros) * (Python committer) Date: 2019-06-21 05:34
Decided to start working on this issue, seems like a solid starting point. After submitting a minor doc contribution, I wanted to also attempt to contribute to some of the easier enhancement requests. 

From my current assessment, it appears that "test_pkgimport", "test_threaded_import", and "threaded_import_hangers" all would appropriately be categorized in test_importlib. Pkgimport attempts to build the package, perform basic filesystem tests, and ensure that run time errors are correctly functioning. "threaded_import" attempts to simultaneously important multiple modules by assigning each one to different threads. The purpose of "threaded_import_hangers" is specifically dependent on "threaded_import", so those two should be grouped together.

While going through test_pkgimport I noticed that it uses the method "random.choose" on lines 17 and 63. However, the standard seems to "random.choice". I could not find any specific mention of random.choose on the python docs, so this appears to be a deprecated method. As far as I'm aware "random.choice" follows current standard and is used far more frequently within the CPython repository. Would it be appropriate to change the "choose" method to "choice"? 

Additionally, as far as naming convention goes, should the name of "test_pkgimport" instead be "test_pkg_import"? This is entirely semantics, but it makes for better consistency with the other file names and is a bit more readable. Also, would it be best to perform all of these changes within separate pull requests, or are they minor enough to be combined into one?
msg346235 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2019-06-21 17:03
> Would it be appropriate to change the "choose" method to "choice"? 

Yep, just make sure the tests still pass before and after the change. :)

> should the name of "test_pkgimport" instead be "test_pkg_import"?

Since things are moving it's fine to rename the file.

> would it be best to perform all of these changes within separate pull requests, or are they minor enough to be combined into one?

Separate are easier to review.
msg346267 - (view) Author: Kyle Stanley (aeros) * (Python committer) Date: 2019-06-22 02:37
> Yep, just make sure the tests still pass before and after the change. :)

Sounds good, the first thing I had done before proposing the change was testing it in an IDE and using some logging to ensure that random.choose and random.choice were providing the same functionality. So hopefully the PR tests will pass as well. 

> Separate are easier to review.

Alright, I'll do each of the file moves in individual PRs and then a separate one for changing random.choice to random.choose. 

Thanks!
msg346444 - (view) Author: Kyle Stanley (aeros) * (Python committer) Date: 2019-06-24 22:10
Created a PR for moving and renaming "test_pkimport". An initial approval was made, now it is awaiting core review (https://github.com/python/cpython/pull/14303).
msg346863 - (view) Author: miss-islington (miss-islington) Date: 2019-06-28 19:37
New changeset 80097e089ba22a42d804e65fbbcf35e5e49eed00 by Miss Islington (bot) (Kyle Stanley) in branch 'master':
bpo-19696: Moved "test_pkgimport.py" to dir "test_importlib" (GH-14303)
https://github.com/python/cpython/commit/80097e089ba22a42d804e65fbbcf35e5e49eed00
msg346864 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2019-06-28 19:38
Thanks for the PR, aeros167! BTW, if you want to open a new issue and modernize the tests to use importlib directly that would be great!
msg346883 - (view) Author: Kyle Stanley (aeros) * (Python committer) Date: 2019-06-29 15:27
> Thanks for the PR, aeros167! BTW, if you want to open a new issue and modernize the tests to use importlib directly that would be great!

Sounds good, I'll definitely look into doing that after finishing up this issue. Was waiting the previous PR to be merged before replacing the deprecated method "random.choose" with "random.choice" in "test_pkg_import.py" and then also moving the other two into test_importlib. 

Thank you very much for the timely feedback. This has been a great experience for learning to contribute to larger open source projects. It's been an aspiration of mine to give back to Python, as it was my first serious programming language 5 years ago. This may be an incredibly minor contribution in the grand scheme of things, but my goal is for it to be the first of many (:
msg346888 - (view) Author: Kyle Stanley (aeros) * (Python committer) Date: 2019-06-29 16:04
Created a new PR replacing the deprecated method "random.choose" with "random.choice" in "test_pkg_import.py" (https://github.com/python/cpython/pull/14466).
msg347091 - (view) Author: Kyle Stanley (aeros) * (Python committer) Date: 2019-07-01 23:35
Created a new PR for moving the last two files "test_threaded_import.py" and "threaded_import_hangers.py" to the directory "test_importlib". (https://github.com/python/cpython/pull/14537)

There were some issues with automatically merging the changes, was the conflict because I attempted to move two files in a single PR? The previous one (which was just moving a single file) did not have any issues with merge conflicts.

This should be the final PR required for this issue, afterwards I'll look into creating a new issue for modernizing the tests.
msg347236 - (view) Author: miss-islington (miss-islington) Date: 2019-07-03 18:22
New changeset 56ec4f1fdedd5b38deb06d94d51dd1a540262e90 by Miss Islington (bot) (Kyle Stanley) in branch 'master':
bpo-19696: Replace deprecated method in "test_import_pkg.py" (GH-14466)
https://github.com/python/cpython/commit/56ec4f1fdedd5b38deb06d94d51dd1a540262e90
msg347259 - (view) Author: Kyle Stanley (aeros) * (Python committer) Date: 2019-07-04 07:40
In order to avoid the merge conflicts, I'm going to move test_threaded_imports.py and threaded_import_hangers.py in separate PRs. Here's the PR for moving test_threaded_imports.py https://github.com/python/cpython/pull/14582.
msg347260 - (view) Author: Kyle Stanley (aeros) * (Python committer) Date: 2019-07-04 07:42
Typo in previous comment: "test_threaded_imports.py" should be "test_threaded_import.py".
msg347517 - (view) Author: Kyle Stanley (aeros) * (Python committer) Date: 2019-07-09 05:00
Opened a new PR for moving "threaded_import_hangers" to "test_importlib/" (PR-14642). This should be the final file in "tests/" to move into "test_importlib/", so the issue can be closed if this PR is merged. I'll create a new issue for modernizing the tests once this issue is closed.
msg347518 - (view) Author: Kyle Stanley (aeros) * (Python committer) Date: 2019-07-09 05:03
Typo: The PR in the above comment should be 14655, not 14642.
msg347757 - (view) Author: miss-islington (miss-islington) Date: 2019-07-12 21:22
New changeset a65c977552507dd19d2cc073fc91ae22cc66bbff by Miss Islington (bot) (Kyle Stanley) in branch 'master':
bpo-19696: Move threaded_import_hangers (GH-14655)
https://github.com/python/cpython/commit/a65c977552507dd19d2cc073fc91ae22cc66bbff
msg347777 - (view) Author: Kyle Stanley (aeros) * (Python committer) Date: 2019-07-13 06:16
The latest PR-14655 moved the last file, "threaded_import_hangers.py" into the "test_importlib" directory. As far as I can tell, there's nothing else which needs to be moved there, so the issue can be closed.
History
Date User Action Args
2019-07-15 18:52:27brett.cannonsetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2019-07-13 06:16:38aerossetmessages: + msg347777
2019-07-12 21:22:25miss-islingtonsetmessages: + msg347757
2019-07-09 05:03:03aerossetmessages: + msg347518
2019-07-09 05:00:15aerossetmessages: + msg347517
2019-07-09 04:23:30aerossetpull_requests: + pull_request14462
2019-07-08 06:57:24aerossetpull_requests: + pull_request14454
2019-07-04 07:42:26aerossetmessages: + msg347260
2019-07-04 07:40:26aerossetmessages: + msg347259
2019-07-04 07:38:49aerossetpull_requests: + pull_request14401
2019-07-03 18:22:54miss-islingtonsetmessages: + msg347236
2019-07-01 23:35:55aerossetmessages: + msg347091
2019-07-01 23:27:07aerossetpull_requests: + pull_request14350
2019-06-29 16:04:59aerossetmessages: + msg346888
2019-06-29 15:56:46aerossetpull_requests: + pull_request14283
2019-06-29 15:27:20aerossetmessages: + msg346883
2019-06-28 19:38:41brett.cannonsetmessages: + msg346864
2019-06-28 19:37:14miss-islingtonsetnosy: + miss-islington
messages: + msg346863
2019-06-24 22:10:26aerossetmessages: + msg346444
2019-06-22 02:45:17aerossetkeywords: + patch
stage: needs patch -> patch review
pull_requests: + pull_request14127
2019-06-22 02:37:49aerossetmessages: + msg346267
2019-06-21 17:03:09brett.cannonsetmessages: + msg346235
2019-06-21 05:34:59aerossetnosy: + aeros
messages: + msg346184
2019-06-17 21:17:27brett.cannonsetmessages: + msg345910
2019-06-16 21:54:05gforcadasetnosy: + gforcada
messages: + msg345783
2013-12-13 21:47:47brett.cannonunlinkissue19704 dependencies
2013-12-13 20:03:36brett.cannonunlinkissue19705 dependencies
2013-11-29 20:41:25Arfreversetnosy: + Arfrever
2013-11-29 16:07:44brett.cannonlinkissue19704 dependencies
2013-11-29 16:07:28brett.cannonlinkissue19705 dependencies
2013-11-22 18:52:50eric.snowsetnosy: + eric.snow
messages: + msg203833
2013-11-22 16:04:45brett.cannoncreate