Hi Troy,
The diff --normal option is a GNU extension and hence not portable to
systems which do not use GNU diff.
See https://ci.apache.org/builders/svn-bb-openbsd/builds/27/steps/Test%20bindings/logs/stdio
which fails with:
trunk/subversion/bindings/swig/python/tests/run_all.py
............................................diff: unknown option -- normal
I would suggest to just drop the --normal option if possible, because it
seems to be the default behaviour for GNU diff and likely also other
implementations.
Another suggestion: Is there a pythonic way to test for the existence
of a 'diff' program, instead of assuming that any system except Windows
has one installed?
Our main test suite has a concept of 'skipped' tests, and if this exists
in the python bindings tests as well it would make sense to report the
test result as 'skipped' instead of 'pass' if no diff tool is available.
Regards,
Stefan
On Fri, Feb 16, 2018 at 05:12:05AM -0000, troycurtisjr_at_apache.org wrote:
> Author: troycurtisjr
> Date: Fri Feb 16 05:12:05 2018
> New Revision: 1824410
>
> URL: http://svn.apache.org/viewvc?rev=1824410&view=rev
> Log:
> Fix Python unit test, fs.SubversionFSTestCase, on Windows.
>
> * subversion/bindings/swig/python/tests/fs.py
> (SubversionFSTestCase.test_diff_repos_paths):
> Update default test case to invoke internal diff implementation and add
> a case for testing calls to the 'diff' executable on non-Windows platforms.
>
> * subversion/bindings/swig/python/svn/fs.py
> (FileDiff.__init__): Add difftemp to track temporary file for cleanup.
> (FileDiff.__del__): Ensure difftemp is cleaned up.
> (File.get_pipe): Add a condition to use the internal diff implementation when
> the diffoptions value is given as None.
>
> Modified:
> subversion/trunk/subversion/bindings/swig/python/svn/fs.py
> subversion/trunk/subversion/bindings/swig/python/tests/fs.py
>
> Modified: subversion/trunk/subversion/bindings/swig/python/svn/fs.py
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/bindings/swig/python/svn/fs.py?rev=1824410&r1=1824409&r2=1824410&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/bindings/swig/python/svn/fs.py (original)
> +++ subversion/trunk/subversion/bindings/swig/python/svn/fs.py Fri Feb 16 05:12:05 2018
> @@ -42,6 +42,7 @@ except ImportError:
> # Python >=3.0
> import builtins
> import svn.core as _svncore
> +import svn.diff as _svndiff
>
>
> def entries(root, path, pool=None):
> @@ -58,6 +59,7 @@ class FileDiff:
>
> self.tempfile1 = None
> self.tempfile2 = None
> + self.difftemp = None
>
> self.root1 = root1
> self.path1 = path1
> @@ -110,27 +112,49 @@ class FileDiff:
> def get_pipe(self):
> self.get_files()
>
> - # use an array for the command to avoid the shell and potential
> - # security exposures
> - cmd = ["diff"] \
> - + self.diffoptions \
> - + [self.tempfile1, self.tempfile2]
> -
> - # open the pipe, and return the file object for reading from the child.
> - p = _subprocess.Popen(cmd, stdout=_subprocess.PIPE, bufsize=-1,
> - close_fds=_sys.platform != "win32")
> - return p.stdout
> + # If diffoptions were provided, then the diff command needs to be
> + # called in preference to using the internal Subversion diff.
> + if self.diffoptions is not None:
> + # use an array for the command to avoid the shell and potential
> + # security exposures
> + cmd = ["diff"] \
> + + self.diffoptions \
> + + [self.tempfile1, self.tempfile2]
> +
> + # open the pipe, and return the file object for reading from the child.
> + p = _subprocess.Popen(cmd, stdout=_subprocess.PIPE, bufsize=-1,
> + close_fds=_sys.platform != "win32")
> + return p.stdout
> +
> + else:
> + if self.difftemp is None:
> + self.difftemp = _tempfile.mktemp()
> +
> + with builtins.open(self.difftemp, "wb") as fp:
> + diffopt = _svndiff.file_options_create()
> + diffobj = _svndiff.file_diff_2(self.tempfile1,
> + self.tempfile2,
> + diffopt)
> +
> + _svndiff.file_output_unified4(fp,
> + diffobj,
> + self.tempfile1,
> + self.tempfile2,
> + None, None,
> + "utf8",
> + None,
> + diffopt.show_c_function,
> + diffopt.context_size,
> + None, None)
> +
> + return builtins.open(self.difftemp, "rb")
>
> def __del__(self):
> # it seems that sometimes the files are deleted, so just ignore any
> # failures trying to remove them
> - if self.tempfile1 is not None:
> - try:
> - _os.remove(self.tempfile1)
> - except OSError:
> - pass
> - if self.tempfile2 is not None:
> - try:
> - _os.remove(self.tempfile2)
> - except OSError:
> - pass
> + for tmpfile in [self.tempfile1, self.tempfile2, self.difftemp]:
> + if tmpfile is not None:
> + try:
> + _os.remove(tmpfile)
> + except OSError:
> + pass
>
> Modified: subversion/trunk/subversion/bindings/swig/python/tests/fs.py
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/bindings/swig/python/tests/fs.py?rev=1824410&r1=1824409&r2=1824410&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/bindings/swig/python/tests/fs.py (original)
> +++ subversion/trunk/subversion/bindings/swig/python/tests/fs.py Fri Feb 16 05:12:05 2018
> @@ -19,7 +19,7 @@
> # under the License.
> #
> #
> -import os, unittest
> +import os, unittest, sys
> from tempfile import mkstemp
> try:
> # Python >=3.0
> @@ -59,7 +59,7 @@ class SubversionFSTestCase(unittest.Test
> """Test diffing of a repository path."""
> tmpfd, self.tmpfile = mkstemp()
>
> - tmpfp = os.fdopen(tmpfd, "w")
> + tmpfp = os.fdopen(tmpfd, "wb")
>
> # Use a unicode file to ensure proper non-ascii handling.
> tmpfp.write(u'⊙_ʘ'.encode('utf8'))
> @@ -83,13 +83,25 @@ class SubversionFSTestCase(unittest.Test
> clientctx)
> self.assertEqual(commitinfo.revision, self.rev + 1)
>
> + # Test standard internal diff
> fdiff = fs.FileDiff(fs.revision_root(self.fs, commitinfo.revision), "/trunk/UniTest.txt",
> - None, None)
> + None, None, diffoptions=None)
>
> diffp = fdiff.get_pipe()
> diffoutput = diffp.read().decode('utf8')
>
> - self.assertTrue(diffoutput.find(u'< ⊙_ʘ') > 0)
> + self.assertTrue(diffoutput.find(u'-⊙_ʘ') > 0)
> +
> + # Test passing diffoptions to an external 'diff' executable.
> + # It is unusual to have the 'diff' tool on Windows, so do not
> + # try the test there.
> + if sys.platform != "win32":
> + fdiff = fs.FileDiff(fs.revision_root(self.fs, commitinfo.revision), "/trunk/UniTest.txt",
> + None, None, diffoptions=['--normal'])
> + diffp = fdiff.get_pipe()
> + diffoutput = diffp.read().decode('utf8')
> +
> + self.assertTrue(diffoutput.find(u'< ⊙_ʘ') > 0)
>
> def suite():
> return unittest.defaultTestLoader.loadTestsFromTestCase(
>
>
Received on 2018-02-17 13:38:23 CET