search

python / cpython

The Python programming language
https://www.python.org
Other
63.35k stars 30.34k forks source link

Merge all (non-syntactic) import-related tests into test_importlib #63895

Closed brettcannon closed 5 years ago

brettcannon commented 10 years ago
BPO 19696
Nosy @brettcannon, @ericsnowcurrently, @miss-islington, @gforcada, @aeros
PRs
  • python/cpython#14303
  • python/cpython#14466
  • python/cpython#14537
  • python/cpython#14582
  • python/cpython#14642
  • python/cpython#14655
    		•

    Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields: ```python assignee = None closed_at = created_at = labels = ['easy', 'type-feature', 'tests'] title = 'Merge all (non-syntactic) import-related tests into test_importlib' updated_at = user = 'https://github.com/brettcannon' ``` bugs.python.org fields: ```python activity = actor = 'brett.cannon' assignee = 'none' closed = True closed_date = closer = 'brett.cannon' components = ['Tests'] creation = creator = 'brett.cannon' dependencies = [] files = [] hgrepos = [] issue_num = 19696 keywords = ['patch', 'easy'] message_count = 20.0 messages = ['203792', '203833', '345783', '345910', '346184', '346235', '346267', '346444', '346863', '346864', '346883', '346888', '347091', '347236', '347259', '347260', '347517', '347518', '347757', '347777'] nosy_count = 6.0 nosy_names = ['brett.cannon', 'Arfrever', 'eric.snow', 'miss-islington', 'gforcada', 'aeros'] pr_nums = ['14303', '14466', '14537', '14582', '14642', '14655'] priority = 'low' resolution = 'fixed' stage = 'resolved' status = 'closed' superseder = None type = 'enhancement' url = 'https://bugs.python.org/issue19696' versions = ['Python 3.5'] ```

    brettcannon commented 10 years ago

    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).

    ericsnowcurrently commented 10 years ago

    +1

    acf241da-f740-4620-b412-fcfeb9e5141b commented 5 years ago

    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:

    Should those be actually moved to test_importlib?

    brettcannon commented 5 years ago

    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).

    aeros commented 5 years ago

    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?

    brettcannon commented 5 years ago

    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.

    aeros commented 5 years ago

    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!

    aeros commented 5 years ago

    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).

    miss-islington commented 5 years ago

    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

    brettcannon commented 5 years ago

    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!

    aeros commented 5 years ago

    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 (:

    aeros commented 5 years ago

    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).

    aeros commented 5 years ago

    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.

    miss-islington commented 5 years ago

    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

    aeros commented 5 years ago

    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.

    aeros commented 5 years ago

    Typo in previous comment: "test_threaded_imports.py" should be "test_threaded_import.py".

    aeros commented 5 years ago

    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.

    aeros commented 5 years ago

    Typo: The PR in the above comment should be 14655, not 14642.

    miss-islington commented 5 years ago

    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

    aeros commented 5 years ago

    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.