Skip to content
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

bpo-18299: Improving eintrdata's test suites #13847

Closed
wants to merge 5 commits into from

Conversation

shihai1991
Copy link
Member

@shihai1991 shihai1991 commented Jun 5, 2019

It would be increase the function's complexity,
but it looks like a unified function.
Lib/test/support/script_helper.py Outdated Show resolved Hide resolved
env.update(env_vars)
cmd_line.extend(args)
proc = subprocess.Popen(cmd_line, stdin=subprocess.PIPE,
stdout=subprocess.PIPE, stderr=subprocess.PIPE,
env=env, cwd=cwd)
env=env, cwd=cwd,
universal_newlines=universal_newlines)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same thing as over.

Copy link
Member Author

Choose a reason for hiding this comment

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

done:)

@shihai1991 shihai1991 changed the title [WIP] bpo-18299: Adding universal_newlines in script_helper like a unified way bpo-18299: Adding universal_newlines in script_helper like a unified way Jun 7, 2019
@mangrisano
Copy link
Contributor

Did you test this change? If not, please provide it.

@mangrisano
Copy link
Contributor

Furthermore, you also need to fix the doc adding the parameter in the signature.

@shihai1991
Copy link
Member Author

Did you test this change? If not, please provide it.
Let us waiting other developer's opinion, if they think this patch is fine, i will replenish it;)

@shihai1991
Copy link
Member Author

@vstinner Hi, Victor. Looks i choose the complicated way of script_helper. Maybe i could find the way which you said to supply popen more easily.

@gpshead
Copy link
Member

gpshead commented Jun 8, 2019

For a new feature in test.support.script_helper it would help to have some motivating examples refactoring some existing test suite to cleaner due to the new API.

@shihai1991
Copy link
Member Author

@gpshead Thanks for review. According your and victor's opinion, I refactor the eintr_tester's test suites.
And pls forget my previous patch;)

@shihai1991 shihai1991 changed the title bpo-18299: Adding universal_newlines in script_helper like a unified way bpo-18299: Improving eintrdata's test suites Jun 13, 2019

@unittest.skipUnless(hasattr(signal, "setitimer"), "requires setitimer()")
class OSEINTRTest(EINTRBaseTest):
""" EINTR tests for the os module. """

def new_sleep_process(self):
code = 'import time; time.sleep(%r)' % self.sleep_time
return self.subprocess(code)
return spawn_python('-c', code)
Copy link
Member

Choose a reason for hiding this comment

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

I don't see how this is beneficial. The point of that method is to avoid having to type '-c' everywhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

This doesn't materially improve the eintrdata test suite and the issue it links to is about script_helper, _assert_python, and text mode to the child. which don't seem to be a problem in eintrdata_tester.py

ok, the spawn_python in script_helper is good enough. Forget this.

@gpshead
Copy link
Member

gpshead commented Jun 13, 2019

This doesn't materially improve the eintrdata test suite and the issue it links to is about script_helper, _assert_python, and text mode to the child. which don't seem to be a problem in eintrdata_tester.py

@gpshead gpshead closed this Jun 13, 2019
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