Hi,
Den fre 22 nov. 2024 kl 14:33 skrev Daniel Shahaf <d...@daniel.shahaf.name>:
> If I force an svnsync test to fail:
>
> [[[
> Index: subversion/tests/cmdline/svnsync_tests.py
> ===================================================================
> --- subversion/tests/cmdline/svnsync_tests.py (revision 1922016)
> +++ subversion/tests/cmdline/svnsync_tests.py (working copy)
> @@ -481,7 +483,7 @@ def delete_revprops(sbox):
> svntest.actions.enable_revprop_changes(sbox.repo_dir)
> exit_code, out, err = svntest.main.run_svn(None,
> 'pdel',
> - '-r', '1',
> + '-r', '0',
> '--revprop',
> 'issue-id',
> sbox.repo_url)
> ]]]
>
> I get the following error:
>
> [[[
> % python3 ./svnsync_tests.py 28 29
> DIFF of raw dumpfiles (including expected differences)
> --- expected
> +++ actual
> W: CWD: subversion/tests/cmdline
> Traceback (most recent call last):
> File "subversion/tests/cmdline/svntest/main.py", line 1989, in run
> rc = self.pred.run(sandbox)
> ^^^^^^^^^^^^^^^^^^^^^^
> File "subversion/tests/cmdline/svntest/testcase.py", line 178, in run
> result = self.func(sandbox)
> ^^^^^^^^^^^^^^^^^^
> File "subversion/tests/cmdline/./svnsync_tests.py", line 493, in
> delete_revprops
> verify_mirror(dest_sbox, expected_contents)
> File "subversion/tests/cmdline/./svnsync_tests.py", line 169, in
> verify_mirror
> svntest.verify.compare_dump_files(
> File "subversion/tests/cmdline/svntest/verify.py", line 855, in
> compare_dump_files
> print(''.join(ndiff(expected, actual)))
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
^ I'll be talking about this line a little further down
> File "/usr/lib/python3.11/difflib.py", line 872, in compare
> yield from g
> File "/usr/lib/python3.11/difflib.py", line 923, in _fancy_replace
> cruncher.set_seq2(bj)
> File "/usr/lib/python3.11/difflib.py", line 248, in set_seq2
> self.__chain_b()
> File "/usr/lib/python3.11/difflib.py", line 289, in __chain_b
> if isjunk(elt):
> ^^^^^^^^^^^
> File "/usr/lib/python3.11/difflib.py", line 1077, in IS_CHARACTER_JUNK
> return ch in ws
> ^^^^^^^^
> TypeError: 'in <string>' requires string as left operand, not int
> FAIL: svnsync_tests.py 28: copy-revprops with removals
> PASS: svnsync_tests.py 29: fd leak during sync from serf to local
> ]]]
>
> So, two things are wrong here:
>
> 1. The test harness doesn't actually print the diff.
>
> That's probably because ndiff() is passed two lists of bytes objects,
> whereas it expects lists of str objects.
>
> The actual diff is:
>
> [[[
> @@ -13,9 +13,13 @@
> b'PROPS-END\n'
> b'\n'
> b'Revision-number: 1\n'
> -b'Prop-content-length: 114\n'
> -b'Content-length: 114\n'
> +b'Prop-content-length: 136\n'
> +b'Content-length: 136\n'
> b'\n'
> +b'K 8\n'
> +b'issue-id\n'
> +b'V 4\n'
> +b'1729\n'
> b'K 10\n'
> b'svn:author\n'
> b'V 8\n'
> ]]]
>
I've tried to track down the root cause. I think the issue was "introduced"
in r1742950 <https://svn.apache.org/r1742950>: "Dumps are binary data,
therefore explicitly use byte strings for all operations on them". But
since most tests succeed - good code quality I assume - compare_dump_files
never reach the ndiff() call in the above traceback and therefor it wasn't
noticed that ndiff() want strings.
I believe the easiest solution is to add list comprehensions decoding each
line:
- print(''.join(ndiff(expected, actual)))
+ print(''.join(ndiff([line.decode('utf-8') for line in expected],
[line.decode('utf-8') for line in actual])))
I've forced the error in another way, by
editing
subversion/tests/cmdline/svnsync_tests_data/delete-revprops.expected.dump
changing cmpilato to cMpilato and running
$ make check TESTS=subversion/tests/cmdline/svnsync_tests.py#28
Before the change I got the same TypeError as above. After the change I get
a failed test with the expected exception.
After reviewing history, I believe the above change is the "obvious fix".
One could argue if decode() should use some other encoding, I don't really
know this. I've committed as r1922041.
>
> 2. The next test was run.
>
> It shouldn't have run, because the exception wasn't just the actual
> output differing from the expected output, but an error in
> determining whether they differ or not.
>
I'm assuming you are looking for something like this:
[[[
Index: subversion/tests/cmdline/svntest/main.py
===================================================================
--- subversion/tests/cmdline/svntest/main.py (revision 1921978)
+++ subversion/tests/cmdline/svntest/main.py (working copy)
@@ -2016,9 +2016,11 @@
logger.error('EXCEPTION: SystemExit(%d), skipping cleanup' % ex.code)
self._print_name(ex.code and 'FAIL: ' or 'PASS: ')
raise
- except:
+ except BaseException as ex:
result = svntest.testcase.RESULT_FAIL
- logger.warn('CWD: %s' % os.getcwd(), exc_info=True)
+ logger.warn('CWD: %s' % os.getcwd(), exc_info=False)
+ logger.warn('Unexpected exception type %s' % type(ex))
+ raise
os.chdir(saved_dir)
exit_code, result_text, result_benignity = self.pred.results(result)
]]]
(Only the "raise" is actually needed here, I added some additional output
to help making it clear that the exception type is unexpected).
After this change, I get:
[[[
$ make check TESTS=subversion/tests/cmdline/svnsync_tests.py
[1/1]
svnsync_tests.py...............................................................................make:
*** [Makefile:553: check] Error 1
]]]
Without this change, the tests complete and it is clearly indicated that
one test failed.
[[[
$ make check TESTS=subversion/tests/cmdline/svnsync_tests.py
[1/1]
svnsync_tests.py...........................................................................................FAILURE
At least one test FAILED, checking /home/daniel/svn_trunk/tests.log
FAIL: svnsync_tests.py 28: copy-revprops with removals
Summary of test results:
30 tests PASSED
1 test FAILED
Python version: 3.11.6.
SUMMARY: Some tests failed
make: *** [Makefile:553: check] Error 1
]]]
I'm not sure I agree the changed behaviour is better. I like the summary
and it is clearly indicated that one test failed. Checking tests.log should
make it clear what failed. If I'd change something, I'd probably just add
the "Unexpected exception" message from the diff above. This would make it
clear in tests.log that we got an unexpected exception:
[[[
DIFF of raw dumpfiles (including expected differences)
--- expected
+++ actual
W: CWD: /home/daniel/svn_trunk/subversion/tests/cmdline
Traceback (most recent call last):
File "/home/daniel/svn_trunk/subversion/tests/cmdline/svntest/main.py",
line 1989, in run
rc = self.pred.run(sandbox)
^^^^^^^^^^^^^^^^^^^^^^
File
"/home/daniel/svn_trunk/subversion/tests/cmdline/svntest/testcase.py", line
178, in run
result = self.func(sandbox)
^^^^^^^^^^^^^^^^^^
File "/home/daniel/svn_trunk/subversion/tests/cmdline/svnsync_tests.py",
line 493, in delete_revprops
verify_mirror(dest_sbox, expected_contents)
File "/home/daniel/svn_trunk/subversion/tests/cmdline/svnsync_tests.py",
line 169, in verify_mirror
svntest.verify.compare_dump_files(
File "/home/daniel/svn_trunk/subversion/tests/cmdline/svntest/verify.py",
line 856, in compare_dump_files
print(''.join(ndiff(expected, actual)))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/lib/python3.11/difflib.py", line 872, in compare
yield from g
File "/usr/lib/python3.11/difflib.py", line 923, in _fancy_replace
cruncher.set_seq2(bj)
File "/usr/lib/python3.11/difflib.py", line 248, in set_seq2
self.__chain_b()
File "/usr/lib/python3.11/difflib.py", line 289, in __chain_b
if isjunk(elt):
^^^^^^^^^^^
File "/usr/lib/python3.11/difflib.py", line 1077, in IS_CHARACTER_JUNK
return ch in ws
^^^^^^^^
TypeError: 'in <string>' requires string as left operand, not int
W: Unexpected exception type <class 'TypeError'>
FAIL: svnsync_tests.py 28: copy-revprops with removals
]]]
Thoughts?
Cheers,
Daniel Sahlberg
