Patchwork [5,of,5,V3] tests: check that procutil.std{out,err}.write() returns correct result

login
register
mail settings
Submitter Manuel Jacob
Date July 14, 2020, 6:41 p.m.
Message ID <b2aabfc8d6800bf6e103.1594752111@tmp>
Download mbox | patch
Permalink /patch/46746/
State Accepted
Headers show

Comments

Manuel Jacob - July 14, 2020, 6:41 p.m.
# HG changeset patch
# User Manuel Jacob <me@manueljacob.de>
# Date 1594291924 -7200
#      Thu Jul 09 12:52:04 2020 +0200
# Node ID b2aabfc8d6800bf6e103b783a245c45e3b1ae4ac
# Parent  49ebcf3b048c7352a4c32b1fd6c793ab99aec246
# EXP-Topic stdio
tests: check that procutil.std{out,err}.write() returns correct result

On Windows, we currently don’t fully test the case when the stream is connected
to a TTY, but we test the child process side by connecting them to NUL, which
is recognized as a TTY by Python. To make the large write test a bit more
useful besides checking that it doesn’t crash, we can check that the write()
method returns the correct result.
Yuya Nishihara - July 15, 2020, 11:14 a.m.
On Tue, 14 Jul 2020 20:41:51 +0200, Manuel Jacob wrote:
> # HG changeset patch
> # User Manuel Jacob <me@manueljacob.de>
> # Date 1594291924 -7200
> #      Thu Jul 09 12:52:04 2020 +0200
> # Node ID b2aabfc8d6800bf6e103b783a245c45e3b1ae4ac
> # Parent  49ebcf3b048c7352a4c32b1fd6c793ab99aec246
> # EXP-Topic stdio
> tests: check that procutil.std{out,err}.write() returns correct result

> -procutil.{stream}.write(b'x' * 1048576)
> +write_result = procutil.{stream}.write(b'x' * 1048576)
> +with open({write_result_fn}, 'w') as write_result_f:
> +    write_result_f.write(str(write_result))

Nit: {write_result_fn!r} can be used instead of repr().

> +            # tempfile.mktemp() is unsafe in general, as a malicious process
> +            # could create the file before we do. But in tests, we're running
> +            # in a controlled environment.
> +            write_result_fn = tempfile.mktemp()

Even so, I think it's better to not use deprecated function which may be
deleted in future version.
Manuel Jacob - July 15, 2020, 12:23 p.m.
On 2020-07-15 13:14, Yuya Nishihara wrote:
> On Tue, 14 Jul 2020 20:41:51 +0200, Manuel Jacob wrote:
>> # HG changeset patch
>> # User Manuel Jacob <me@manueljacob.de>
>> # Date 1594291924 -7200
>> #      Thu Jul 09 12:52:04 2020 +0200
>> # Node ID b2aabfc8d6800bf6e103b783a245c45e3b1ae4ac
>> # Parent  49ebcf3b048c7352a4c32b1fd6c793ab99aec246
>> # EXP-Topic stdio
>> tests: check that procutil.std{out,err}.write() returns correct result
> 
>> -procutil.{stream}.write(b'x' * 1048576)
>> +write_result = procutil.{stream}.write(b'x' * 1048576)
>> +with open({write_result_fn}, 'w') as write_result_f:
>> +    write_result_f.write(str(write_result))
> 
> Nit: {write_result_fn!r} can be used instead of repr().
> 
>> +            # tempfile.mktemp() is unsafe in general, as a malicious 
>> process
>> +            # could create the file before we do. But in tests, we're 
>> running
>> +            # in a controlled environment.
>> +            write_result_fn = tempfile.mktemp()
> 
> Even so, I think it's better to not use deprecated function which may 
> be
> deleted in future version.

According to the module documentation, the file created by the more 
secure (for general use) variants can’t be opened twice, but that’s 
exactly the functionality we need. For this reason, I don’t think 
tempfile.mktemp() will be deleted. But I can try to find some hack.
Yuya Nishihara - July 15, 2020, 12:32 p.m.
On Wed, 15 Jul 2020 14:23:17 +0200, Manuel Jacob wrote:
> On 2020-07-15 13:14, Yuya Nishihara wrote:
> > On Tue, 14 Jul 2020 20:41:51 +0200, Manuel Jacob wrote:
> >> # HG changeset patch
> >> # User Manuel Jacob <me@manueljacob.de>
> >> # Date 1594291924 -7200
> >> #      Thu Jul 09 12:52:04 2020 +0200
> >> # Node ID b2aabfc8d6800bf6e103b783a245c45e3b1ae4ac
> >> # Parent  49ebcf3b048c7352a4c32b1fd6c793ab99aec246
> >> # EXP-Topic stdio
> >> tests: check that procutil.std{out,err}.write() returns correct result
> > 
> >> -procutil.{stream}.write(b'x' * 1048576)
> >> +write_result = procutil.{stream}.write(b'x' * 1048576)
> >> +with open({write_result_fn}, 'w') as write_result_f:
> >> +    write_result_f.write(str(write_result))
> > 
> > Nit: {write_result_fn!r} can be used instead of repr().
> > 
> >> +            # tempfile.mktemp() is unsafe in general, as a malicious 
> >> process
> >> +            # could create the file before we do. But in tests, we're 
> >> running
> >> +            # in a controlled environment.
> >> +            write_result_fn = tempfile.mktemp()
> > 
> > Even so, I think it's better to not use deprecated function which may 
> > be
> > deleted in future version.
> 
> According to the module documentation, the file created by the more 
> secure (for general use) variants can’t be opened twice, but that’s 
> exactly the functionality we need. For this reason, I don’t think 
> tempfile.mktemp() will be deleted. But I can try to find some hack.

IIUC, we can just create an empty temp file (and close it immediately)
to reserve the file name.

Patch

diff --git a/tests/test-stdio.py b/tests/test-stdio.py
--- a/tests/test-stdio.py
+++ b/tests/test-stdio.py
@@ -10,6 +10,7 @@ 
 import signal
 import subprocess
 import sys
+import tempfile
 import unittest
 
 from mercurial import pycompat
@@ -41,7 +42,9 @@ 
 
 signal.signal(signal.SIGINT, lambda *x: None)
 dispatch.initstdio()
-procutil.{stream}.write(b'x' * 1048576)
+write_result = procutil.{stream}.write(b'x' * 1048576)
+with open({write_result_fn}, 'w') as write_result_f:
+    write_result_f.write(str(write_result))
 '''
 
 
@@ -109,6 +112,7 @@ 
         rwpair_generator,
         check_output,
         python_args=[],
+        post_child_check=None,
     ):
         assert stream in ('stdout', 'stderr')
         with rwpair_generator() as (stream_receiver, child_stream), open(
@@ -130,6 +134,8 @@ 
             finally:
                 retcode = proc.wait()
             self.assertEqual(retcode, 0)
+            if post_child_check is not None:
+                post_child_check()
 
     def _test_buffering(
         self, stream, rwpair_generator, expected_output, python_args=[]
@@ -194,13 +200,39 @@ 
                 _readall(stream_receiver, 131072, buf), b'x' * 1048576
             )
 
-        self._test(
-            TEST_LARGE_WRITE_CHILD_SCRIPT.format(stream=stream),
-            stream,
-            rwpair_generator,
-            check_output,
-            python_args,
-        )
+        def post_child_check():
+            with open(write_result_fn, 'r') as write_result_f:
+                write_result_str = write_result_f.read()
+            if pycompat.ispy3:
+                # On Python 3, we test that the correct number of bytes is
+                # claimed to have been written.
+                expected_write_result_str = '1048576'
+            else:
+                # On Python 2, we only check that the large write does not
+                # crash.
+                expected_write_result_str = 'None'
+            self.assertEqual(write_result_str, expected_write_result_str)
+
+        try:
+            # tempfile.mktemp() is unsafe in general, as a malicious process
+            # could create the file before we do. But in tests, we're running
+            # in a controlled environment.
+            write_result_fn = tempfile.mktemp()
+            self._test(
+                TEST_LARGE_WRITE_CHILD_SCRIPT.format(
+                    stream=stream, write_result_fn=repr(write_result_fn)
+                ),
+                stream,
+                rwpair_generator,
+                check_output,
+                python_args,
+                post_child_check=post_child_check,
+            )
+        finally:
+            try:
+                os.unlink(write_result_fn)
+            except OSError:
+                pass
 
     def test_large_write_stdout_devnull(self):
         self._test_large_write('stdout', _devnull)