Skip to content

bpo-40275: Add os_helper submodule in test.support #20732

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
shihai1991 wants to merge 6 commits into from
Closed

bpo-40275: Add os_helper submodule in test.support #20732

shihai1991 wants to merge 6 commits into from

Conversation

@shihai1991
Copy link
Member

@shihai1991 shihai1991 commented Jun 8, 2020 •
edited
Loading

Import the following vars, functions, classes in os_helper:

  • rmtree()
  • unlink()
  • rmdir()
  • FS_NONASCII
  • TESTFN_UNICODE, TESTFN_NONASCII, TESTFN_UNENCODABLE, TESTFN_UNDECODABLE
  • SAVEDCWD
  • temp_dir()
  • change_cwd()
  • temp_cwd()
  • temp_umask()
  • create_empty_file()
  • make_bad_fd()
  • EnvironmentVarGuard
  • can_symlink(), skip_unless_symlink()
  • can_xattr(), skip_unless_xattr()
  • fs_is_case_insensitive()
  • findfile
  • fd_count()
  • FakePath

https://bugs.python.org/issue40275

@shihai1991 shihai1991 mentioned this pull request Jun 8, 2020
Closed
@shihai1991
Copy link
Member Author

shihai1991 commented Jun 8, 2020

@vstinner Hi, victor. Pls have a look again when you have free time.

@brettcannon brettcannon requested review from vstinner and removed request for brettcannon June 8, 2020 18:55
@vstinner
Copy link
Member

vstinner commented Jun 8, 2020

For the first PR (this one), please leave support/__init__.py unchanged: only import names in os_helper. Also, please leave test_xxx.py unchanged.

@vstinner
Copy link
Member

vstinner commented Jun 8, 2020

For example, os_helper.py would contain something like:

from test.support import TESTFN
...

@shihai1991
Copy link
Member Author

shihai1991 commented Jun 9, 2020

For example, os_helper.py would contain something like:

from test.support import TESTFN
...

Oh, My logic is the opposite. IMHO, it is same behavior, no?

@vstinner
Copy link
Member

vstinner commented Jun 9, 2020

Brett and me explained to you that a PR modifying 100+ files is a bad idea.

Adding a new submodule which only imports existing functions avoids modifying any existing test and so it becomes possible to convert tests slowly, one by one, or 20 by 20.

@shihai1991
Copy link
Member Author

shihai1991 commented Jun 9, 2020 •
edited
Loading

Brett and me explained to you that a PR modifying 100+ files is a bad idea.

Pls forgive me, I have relealized that it's not easy to review and noisy too many developers.

Adding a new submodule which only imports existing functions avoids modifying any existing test and so it becomes possible to convert tests slowly, one by one, or 20 by 20.

IMO: a new submodule which only imports existing functions same as move existing functions in new submodule and reimport those existing functions in test.support

Oh, wait. You are right. Some testcase use inner functions of test.support will be affected.

vstinner
vstinner reviewed Jun 9, 2020
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

move existing functions in new submodule and reimport those existing functions in test.support

Right. I tried to check if there are remaining functions in support/init.py which should be moved to os_helper. But since functions are not moved, it's not easy.

Would you mind to create a second PR which moves functions but adds aliases in support/init.py? It sounds like an even better approach.

Doc/library/test.rst

.. data:: SAVEDCWD

Set to :func:`os.getcwd`.
Copy link
Member

@vstinner vstinner Jun 9, 2020

Choose a reason for hiding this comment

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

Side note: I'm not sure why it exists in test.support. It seems like only test.libregrtest uses it. Maybe it should be moved (later) to test.libregrtest.

Copy link
Member Author

@shihai1991 shihai1991 Jun 12, 2020

Choose a reason for hiding this comment

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

Hm, I checked this detail. Looks like the describe info is right. It points that SAVEDCWD=os.getcwd()

@vstinner
Copy link
Member

vstinner commented Jun 9, 2020

IMO PR #20765 is the right way to fix this issue.

@vstinner vstinner closed this Jun 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner left review comments

@encukou encukou Awaiting requested review from encukou

@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently

@ncoghlan ncoghlan Awaiting requested review from ncoghlan

@warsaw warsaw Awaiting requested review from warsaw

Assignees

No one assigned

Labels

awaiting review skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@shihai1991 @vstinner @the-knights-who-say-ni @bedevere-bot