FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-10572: Move test sub-packages to Lib/test by idomic · Pull Request #18524 · python/cpython · GitHub

/ cpython Public

bpo-10572: Move test sub-packages to Lib/test - #18524

Closed
idomic wants to merge 30 commits into
python:masterfrom
idomic:bpo-10572-reArranging_Lib_tests
Closed

bpo-10572: Move test sub-packages to Lib/test#18524
idomic wants to merge 30 commits into
python:masterfrom
idomic:bpo-10572-reArranging_Lib_tests

Conversation

idomic commented Feb 16, 2020
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

idomic force-pushed the bpo-10572-reArranging_Lib_tests branch from 746c83b to 36b41eb Compare February 16, 2020 18:20

ned-deily left a comment
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Please make sure that tests still run correctly when running from an installed instance of Python, not just from a development build directory. The various test directories are installed in the main Makefile (Makefile.pre.in) libinstall rule. I think you'll find that many tests are now broken. See the LIBSUBDIRS variable in the Makefile. (I see that the dev guide does not refer to this topic so I've opened a devguide issue about it: python/devguide#573)

Copy link
Copy Markdown

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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

idomic commented Feb 16, 2020

Copy link
Copy Markdown
Contributor Author

Thanks @ned-deily , I've updated the makefile and adding a News file.
I have made the requested changes; please review again

Copy link
Copy Markdown

Thanks for making the requested changes!

@ned-deily: please review the changes made to this pull request.

idomic requested a review from a team as a code owner February 16, 2020 23:10

idomic commented Feb 17, 2020

Copy link
Copy Markdown
Contributor Author

Ok I fixed all of the tests expect 1 in lib2to3 in which I'm not really sure what's going on.

idomic commented Feb 23, 2020

Copy link
Copy Markdown
Contributor Author

When looking on the test that's failing:

test_write_filtered_python_package (test.test_zipfile.PyZipFileTests) ... File "/Users/runner/runners/2.164.0/work/cpython/cpython/Lib/test/lib2to3_tests/data/bom.py", line 2
print "BOM BOOM!"

It looks like it uses print syntax of python 2.7 which is not relevant anymore, any idea why is it there? I suspect in the master it was just not running and because now it's in the tests folder it's running by default.

I have made the requested changes; please review again

Copy link
Copy Markdown

Thanks for making the requested changes!

@ned-deily: please review the changes made to this pull request.

brettcannon self-requested a review February 27, 2020 23:08

Copy link
Copy Markdown
Member

@idomic it looks like it's a test file for the lib2to3 test to use. There's probably a test that reads that file, runs lib2to3 on it, and checks the results. So that file is very likely there on purpose and meant to be Python 2 syntax.

Copy link
Copy Markdown
Member

@idomic you also have conflicts. It might be easier to do one PR per package so lessen the chances of merge conflicts occurring.

brettcannon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The merge conflicts need to be fixed, and I would strongly suggest breaking this PR up into single PRs per package.

Copy link
Copy Markdown

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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

idomic commented Mar 15, 2020

Copy link
Copy Markdown
Contributor Author

The merge conflicts need to be fixed, and I would strongly suggest breaking this PR up into single PRs per package.

I'll submit a new PR with 1 package and will tag you and Ned

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants


Back | FazBrowse Home | New Git URL