You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by fu...@apache.org on 2020/12/10 14:45:44 UTC
svn commit: r1884279 - in /subversion/trunk: build/
subversion/tests/cmdline/svntest/
Author: futatuki
Date: Thu Dec 10 14:45:44 2020
New Revision: 1884279
URL: http://svn.apache.org/viewvc?rev=1884279&view=rev
Log:
Ensure close file descriptors by using context manager in test suite.
Generally, it is true that as file descriptors are closed when their
underlying objects are deleted, we don't need close them explicitly.
However if a Python language implementation that uses other strategy
than reference count model, there is no warranty it will happen
immediately after when those objects lose the last reference. So we
use context manager to ensure close() is called immediately after
those objects to be unnecessary. This helps us to run 'make check'
with PyPy even smaler limit of number of open files, although we
don't recommend use it because it is much slower than CPython for
this purpose.
* build/run_tests.py
(TestHarness._init_c_tests): Use context manager to write file.
(TestHarness.Job.execute,
TestHarness.CollectingThread._count_c_tests,
Testharness.CollectingThread._count_py_tests,):
Use context manager for Popen object.
* subversion/tests/cmdline/svntest/actions.py
(load_repo, load_dumpfile): Use context manager to read file contents.
* subversion/tests/cmdline/svntest/main.py
(trust_ssl_cert): Use context manager to read file contents.
* subversion/tests/cmdline/svntest/sandbox.py
(Sandbox._ensure_authz get_content): New function.
(Sandbox._ensure_authz): Use it.
* subversion/tests/cmdline/svntest/tree.py
(get_text): Use context manager to read file contents.
* subversion/tests/cmdline/svntest/wc.py
(State.from_wc): Use context manager to read file contents.
Modified:
subversion/trunk/build/run_tests.py
subversion/trunk/subversion/tests/cmdline/svntest/actions.py
subversion/trunk/subversion/tests/cmdline/svntest/main.py
subversion/trunk/subversion/tests/cmdline/svntest/sandbox.py
subversion/trunk/subversion/tests/cmdline/svntest/tree.py
subversion/trunk/subversion/tests/cmdline/svntest/wc.py
Modified: subversion/trunk/build/run_tests.py
URL: http://svn.apache.org/viewvc/subversion/trunk/build/run_tests.py?rev=1884279&r1=1884278&r2=1884279&view=diff
==============================================================================
--- subversion/trunk/build/run_tests.py (original)
+++ subversion/trunk/build/run_tests.py Thu Dec 10 14:45:44 2020
@@ -215,8 +215,9 @@ class TestHarness:
authzparent = os.path.join(self.builddir, subdir)
if not os.path.exists(authzparent):
os.makedirs(authzparent);
- open(os.path.join(authzparent, 'authz'), 'w').write('[/]\n'
- '* = rw\n')
+ with open(os.path.join(authzparent, 'authz'), 'w') as fp:
+ fp.write('[/]\n'
+ '* = rw\n')
# ### Support --repos-template
if self.opts.list_tests is not None:
@@ -363,15 +364,15 @@ class TestHarness:
def execute(self, harness):
start_time = datetime.now()
- prog = subprocess.Popen(self._command_line(harness),
+ with subprocess.Popen(self._command_line(harness),
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
- cwd=self.progdir)
+ cwd=self.progdir) as prog:
- self.stdout_lines = prog.stdout.readlines()
- self.stderr_lines = prog.stderr.readlines()
- prog.wait()
- self.result = prog.returncode
+ self.stdout_lines = prog.stdout.readlines()
+ self.stderr_lines = prog.stderr.readlines()
+ prog.wait()
+ self.result = prog.returncode
self.taken = datetime.now() - start_time
class CollectingThread(threading.Thread):
@@ -389,21 +390,23 @@ class TestHarness:
def _count_c_tests(self, progabs, progdir, progbase):
'Run a c test, escaping parameters as required.'
cmdline = [ progabs, '--list' ]
- prog = subprocess.Popen(cmdline, stdout=subprocess.PIPE, cwd=progdir)
- lines = prog.stdout.readlines()
- self.result.append(TestHarness.Job(len(lines) - 2, False, progabs,
- progdir, progbase))
- prog.wait()
+ with subprocess.Popen(cmdline, stdout=subprocess.PIPE,
+ cwd=progdir) as prog:
+ lines = prog.stdout.readlines()
+ self.result.append(TestHarness.Job(len(lines) - 2, False, progabs,
+ progdir, progbase))
+ prog.wait()
def _count_py_tests(self, progabs, progdir, progbase):
'Run a c test, escaping parameters as required.'
cmdline = [ sys.executable, progabs, '--list' ]
- prog = subprocess.Popen(cmdline, stdout=subprocess.PIPE, cwd=progdir)
- lines = prog.stdout.readlines()
-
- for i in range(0, len(lines) - 2):
- self.result.append(TestHarness.Job(i + 1, True, progabs,
- progdir, progbase))
+ with subprocess.Popen(cmdline, stdout=subprocess.PIPE,
+ cwd=progdir) as prog:
+ lines = prog.stdout.readlines()
+
+ for i in range(0, len(lines) - 2):
+ self.result.append(TestHarness.Job(i + 1, True, progabs,
+ progdir, progbase))
prog.wait()
def run(self):
Modified: subversion/trunk/subversion/tests/cmdline/svntest/actions.py
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/svntest/actions.py?rev=1884279&r1=1884278&r2=1884279&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/cmdline/svntest/actions.py (original)
+++ subversion/trunk/subversion/tests/cmdline/svntest/actions.py Thu Dec 10 14:45:44 2020
@@ -507,7 +507,8 @@ def load_repo(sbox, dumpfile_path = None
normalize_props = False):
"Loads the dumpfile into sbox"
if not dump_str:
- dump_str = open(dumpfile_path, "rb").read()
+ with open(dumpfile_path, "rb") as fp:
+ dump_str = fp.read()
# Create a virgin repos and working copy
main.safe_rmtree(sbox.repo_dir, 1)
@@ -2076,7 +2077,9 @@ def get_wc_base_rev(wc_dir):
def load_dumpfile(filename):
"Return the contents of the FILENAME assuming that it is a dump file"
- return open(filename, "rb").readlines()
+ with open(filename, "rb") as fp:
+ dump_str = fp.readlines()
+ return dump_str
def hook_failure_message(hook_name):
"""Return the error message that the client prints for failure of the
Modified: subversion/trunk/subversion/tests/cmdline/svntest/main.py
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/svntest/main.py?rev=1884279&r1=1884278&r2=1884279&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/cmdline/svntest/main.py (original)
+++ subversion/trunk/subversion/tests/cmdline/svntest/main.py Thu Dec 10 14:45:44 2020
@@ -742,9 +742,9 @@ def trust_ssl_cert(cfgdir, ssl_cert, ssl
"""
cert_rep = ''
- fp = open(ssl_cert, 'r')
- for line in fp.readlines()[1:-1]:
- cert_rep = cert_rep + line.strip()
+ with open(ssl_cert, 'r') as fp:
+ for line in fp.readlines()[1:-1]:
+ cert_rep = cert_rep + line.strip()
parsed_url = urlparse(ssl_url)
netloc_url = '%s://%s' % (parsed_url.scheme, parsed_url.netloc)
Modified: subversion/trunk/subversion/tests/cmdline/svntest/sandbox.py
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/svntest/sandbox.py?rev=1884279&r1=1884278&r2=1884279&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/cmdline/svntest/sandbox.py (original)
+++ subversion/trunk/subversion/tests/cmdline/svntest/sandbox.py Thu Dec 10 14:45:44 2020
@@ -167,12 +167,17 @@ class Sandbox:
def _ensure_authz(self):
"make sure the repository is accessible"
+ def get_content(f):
+ with open(f, 'r') as fp:
+ content = fp.read()
+ return content
+
if self.repo_url.startswith("http"):
default_authz = "[/]\n* = rw\n"
if (svntest.main.options.parallel == 0
and (not os.path.isfile(self.authz_file)
- or open(self.authz_file,'r').read() != default_authz)):
+ or get_content(self.authz_file) != default_authz)):
tmp_authz_file = os.path.join(svntest.main.work_dir, "authz-" + self.name)
with open(tmp_authz_file, 'w') as f:
Modified: subversion/trunk/subversion/tests/cmdline/svntest/tree.py
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/svntest/tree.py?rev=1884279&r1=1884278&r2=1884279&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/cmdline/svntest/tree.py (original)
+++ subversion/trunk/subversion/tests/cmdline/svntest/tree.py Thu Dec 10 14:45:44 2020
@@ -553,7 +553,9 @@ def get_text(path):
if not os.path.isfile(path):
return None
- return open(path, 'r').read()
+ with open(path, 'r') as fp:
+ content = fp.read()
+ return content
def get_child(node, name):
Modified: subversion/trunk/subversion/tests/cmdline/svntest/wc.py
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/svntest/wc.py?rev=1884279&r1=1884278&r2=1884279&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/cmdline/svntest/wc.py (original)
+++ subversion/trunk/subversion/tests/cmdline/svntest/wc.py Thu Dec 10 14:45:44 2020
@@ -687,11 +687,11 @@ class State:
if os.path.isfile(node):
try:
if keep_eol_style:
-
- contents = io.open(node, 'r', newline='',
- encoding='utf-8').read()
+ with io.open(node, 'r', newline='', encoding='utf-8') as fp:
+ contents = fp.read()
else:
- contents = io.open(node, 'r', encoding='utf-8').read()
+ with io.open(node, 'r', encoding='utf-8') as fp:
+ contents = fp.read()
if not isinstance(contents, str):
# Python 2: contents is read as an unicode object,
# but we expect it is a str.
@@ -699,7 +699,8 @@ class State:
except:
# If the file contains non UTF-8 character, we treat its
# content as binary represented as a bytes object.
- contents = open(node, 'rb').read()
+ with open(node, 'rb') as fp:
+ contents = fp.read()
else:
contents = None
desc[repos_join(parent, name)] = StateItem(contents=contents)
Re: svn commit: r1884279 - in /subversion/trunk: build/
subversion/tests/cmdline/svntest/
Posted by Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>.
On 2020/12/12 12:33, Daniel Shahaf wrote:
> futatuki@apache.org wrote on Thu, 10 Dec 2020 14:45 +00:00:
<snip all>
Thank you for the review. I've replaced the commit message.
Cheers,
--
Yasuhito FUTATSUKI <fu...@yf.bsclub.org>
Re: svn commit: r1884279 - in /subversion/trunk: build/ subversion/tests/cmdline/svntest/
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
futatuki@apache.org wrote on Thu, 10 Dec 2020 14:45 +00:00:
> Ensure close file descriptors by using context manager in test suite.
Suggest instead:
test suite: Ensure file descriptors are closed in a timely manner, using context managers.
I.e.:
- Use "$area: $description" format
- Fix ungrammatical use of "close"
(alternative: "Ensure timely closure of file descriptors")
And since I'm reviewing already, a few more nits:
> Generally, it is true that as file descriptors are closed when their
> underlying objects are deleted, we don't need close them explicitly.
> However if a Python language implementation that uses other strategy
> than reference count model, there is no warranty it will happen
"if a Python language implementation uses a strategy other than reference counting"
> immediately after when those objects lose the last reference. So we
> use context manager to ensure close() is called immediately after
> those objects to be unnecessary. This helps us to run 'make check'
> with PyPy even smaler limit of number of open files, although we
s/even/under an even/
s/smaler/lower/ (the adjective modifies "limit", not "number")
Consider dropping the symbolic name (RLIMIT_NOFILE) into that sentence somewhere.
> don't recommend use it because it is much slower than CPython for
s/use it/use of it|to use it/
s/it/${whatever the pronoun refers to}/
> this purpose.
No comments on the diff itself, but I only skimmed it.
Cheers,
Daniel
Re: svn commit: r1884279 - in /subversion/trunk: build/ subversion/tests/cmdline/svntest/
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
futatuki@apache.org wrote on Thu, 10 Dec 2020 14:45 +00:00:
> Ensure close file descriptors by using context manager in test suite.
Suggest instead:
test suite: Ensure file descriptors are closed in a timely manner, using context managers.
I.e.:
- Use "$area: $description" format
- Fix ungrammatical use of "close"
(alternative: "Ensure timely closure of file descriptors")
And since I'm reviewing already, a few more nits:
> Generally, it is true that as file descriptors are closed when their
> underlying objects are deleted, we don't need close them explicitly.
> However if a Python language implementation that uses other strategy
> than reference count model, there is no warranty it will happen
"if a Python language implementation uses a strategy other than reference counting"
> immediately after when those objects lose the last reference. So we
> use context manager to ensure close() is called immediately after
> those objects to be unnecessary. This helps us to run 'make check'
> with PyPy even smaler limit of number of open files, although we
s/even/under an even/
s/smaler/lower/ (the adjective modifies "limit", not "number")
Consider dropping the symbolic name (RLIMIT_NOFILE) into that sentence somewhere.
> don't recommend use it because it is much slower than CPython for
s/use it/use of it|to use it/
s/it/${whatever the pronoun refers to}/
> this purpose.
No comments on the diff itself, but I only skimmed it.
Cheers,
Daniel