bpo-33671: Replace platform test for darwin with support.MACOS by 4383 · Pull Request #7875 · python/cpython · GitHub
Skip to content

bpo-33671: Replace platform test for darwin with support.MACOS - #7875

Closed
4383 wants to merge 1 commit into
python:masterfrom
4383:support_ms_windows
Closed

4383 wants to merge 1 commit into
python:masterfrom
4383:support_ms_windows

Conversation

@4383

@4383 4383 commented Jun 23, 2018

Copy link
Copy Markdown
Contributor

Summary

Replace all platform test on darwin system with support.MACOS introduced by #7800

This pull request is focused on darwin, win will be replaced in another pull request.

https://bugs.python.org/issue33671

@4383
4383 requested review from a team, 1st1, asvetlov and gpshead as code owners June 23, 2018 13:47
@the-knights-who-say-ni

Copy link
Copy Markdown

@4383

4383 commented Jun 23, 2018

Copy link
Copy Markdown
Contributor Author

Signed CLA :)

Comment thread Lib/test/test_logging.py Outdated

@pablogsal pablogsal Jun 23, 2018

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.

The tests are failing because MACOS is not imported (You need to use `support.MACOS).

@pablogsal

Copy link
Copy Markdown
Member

Hi @4383 and thank you for your contribution!

The CI is failing right now with this error:


ERROR: test_path_objects (test.test_logging.HandlerTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/python/cpython/Lib/test/test_logging.py", line 616, in test_path_objects
    if MACOS or sys.platform == 'linux':
NameError: name 'MACOS' is not defined

Notice that to avoid waiting for the CI to fail in the PR you can run your tests locally. For example:

./configure --with-pydebug && make && make test

@4383
4383 force-pushed the support_ms_windows branch from 7137a90 to a77fcff Compare June 23, 2018 21:11
@4383
4383 force-pushed the support_ms_windows branch from a77fcff to 519fd6d Compare June 23, 2018 21:28
@ned-deily

Copy link
Copy Markdown
Member

Thanks for the contribution. But I think this or any similar changes should not be merged until we've further discussed the implications of #7800 itself.

@4383

4383 commented Jun 23, 2018

Copy link
Copy Markdown
Contributor Author

Hi @ned-deily, no problems, I can maintain these changes up-to-date during the time you need to measure implications of #7800 :)

Hi @pablogsal, thanks for your help, I pushed too hastily. Errors fixed :)

@ned-deily

Copy link
Copy Markdown
Member

@ned-deily ned-deily closed this Jun 26, 2018
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