-
-
Notifications
You must be signed in to change notification settings - Fork 33.1k
gh-131178: Add tests for sysconfig
command-line interface
#139934
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 to not documenting --generate-posix-vars
. Other comments inline
check=True | ||
) | ||
self.assertTrue(output.returncode == 0) | ||
self.assertTrue(output.stdout.startswith("Platform: ")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docs say that all these values should be present in the text, could we check for them in the test? That way the test validates they're there (and if the output changes, know it needs to change)
get_platform(), get_python_version(), get_path() and get_config_vars().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will add a stricter test case by mocking an output and directly compare the mock output and the real one.
|
||
class CommandLineTests(unittest.TestCase): | ||
def test_config_output(self): | ||
output = subprocess.run( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I can see in the base issue the pattern for these tests is to use contextlib.redirect_stdout
+ call the function which implements main directly (sysconfig._main
) rather than run / use a subprocess.
See for example: https://github.com/python/cpython/pull/131275/files#diff-eabc91c9e7a2586ffc6ca849d6636e74e28825a4515745c280b045dbdf857e39R723-R727
That should also fix the WASI check which is currently failing with: OSError: [Errno 58] wasi does not support processes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! Thank you. I am really stuck on how to fix the WASI test.
Actually we've already got this for the CLI test:
So we don't need to change things(why are the CLI test mixing with others XD) .Sorry that I overlook this. |
As doc points out, we can use
sysconfig
lib directly as a script to obtain configs.This PR add tests for this feature.
PS: I notice that the CLI actually has a
--generare-posix-vars
attribute that is undocumented. I don't think we need to add test for that (maybe this is an internal function or what. If not I think we can add document for this later)