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

tools,test: use Execute instead of check_output #17381

Merged
merged 1 commit into from
Nov 30, 2017

Conversation

refack
Copy link
Contributor

@refack refack commented Nov 29, 2017

subprocess.check_output (f31cf56) is a python2.7 only feature. Using Execute allows
keeping python2.6 compatibility

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

tools,test

@refack refack added the regression Issues related to regressions. label Nov 29, 2017
@nodejs-github-bot nodejs-github-bot added test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. labels Nov 29, 2017
@refack
Copy link
Contributor Author

refack commented Nov 29, 2017

@refack refack added the fast-track PRs that do not need to wait for 48 hours to land. label Nov 29, 2017
@refack
Copy link
Contributor Author

refack commented Nov 29, 2017

Seems like this is the safest way to restore CI to green, so we should to fast-track this.

@refack
Copy link
Contributor Author

refack commented Nov 29, 2017

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Rubber-stamp LGTM

@Trott Trott added the python PRs and issues that require attention from people who are familiar with Python. label Nov 29, 2017
@refack refack mentioned this pull request Nov 29, 2017
Copy link
Contributor

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

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

Rubber Stamp LGTM including fast-track

tools/test.py Outdated
if "fips" in subprocess.check_output([vm, "-p",
"process.versions.openssl"]):
ssl_ver = Execute([vm, "-p", "process.versions.openssl"], context).stdout
if "fips" in ssl_ver:
Copy link
Member

Choose a reason for hiding this comment

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

Since you're here, can you use single quotes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack

@gibfahn
Copy link
Member

gibfahn commented Nov 29, 2017

+1 on fast-track

@refack
Copy link
Contributor Author

refack commented Nov 29, 2017

subprocess.check_output is a python2.7 only feature. Using Execute
allows keeping python2.6 compatibility

PR-URL: nodejs#17381
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@refack refack merged commit fff3792 into nodejs:master Nov 30, 2017
@refack refack deleted the improve-testpy-subprocess branch November 30, 2017 01:36
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
subprocess.check_output is a python2.7 only feature. Using Execute
allows keeping python2.6 compatibility

PR-URL: #17381
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
subprocess.check_output is a python2.7 only feature. Using Execute
allows keeping python2.6 compatibility

PR-URL: #17381
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Dec 12, 2017
gibfahn pushed a commit that referenced this pull request Dec 19, 2017
subprocess.check_output is a python2.7 only feature. Using Execute
allows keeping python2.6 compatibility

PR-URL: #17381
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@gibfahn
Copy link
Member

gibfahn commented Dec 19, 2017

Backport requested here: #16329 (comment)

@gibfahn gibfahn mentioned this pull request Dec 20, 2017
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
subprocess.check_output is a python2.7 only feature. Using Execute
allows keeping python2.6 compatibility

PR-URL: #17381
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fast-track PRs that do not need to wait for 48 hours to land. python PRs and issues that require attention from people who are familiar with Python. regression Issues related to regressions. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants