Ramkumar Ramachandra wrote on Sun, Sep 19, 2010 at 17:53:12 +0530:
> Hi,
>
> I thought this patch might need a little bit of special attention. Let
> me know quickly if something is obviously wrong with it- as far as I
> know, all tests pass and this patch works as expected. I'm going ahead
> and adding lots of tests and fixing whatever bugs there may be shortly.
>
> [[[
> Add some testing infrastructure to exclude comparison of certain lines
> specified by a regular expression so that svnrdump dump tests can
> report actual failures. Exercise this infrastructure in
> svnrdump_tests.py. This is necessary because there is some mismatch
> between the headers that `svnadmin dump` and `svnrdump dump` are able
> to provide.
>
> * subversion/tests/cmdline/svntest/verify.py
>
> (ExpectedOutput.matches_except): Write new function that resembling
> ExpectedOutput.display_lines with an extra except_re argument. It
> compares EXPECTED and ACTUAL on an exact line-by-line basis,
> excluding lines that match the regular expression specified in
> except_re.
>
> (ExceptedOutput.matches): Modify function to accept one more
> optional argument: except_re. Call matches_except if except_re is
> not None.
>
> (compare_and_display_lines): Take an extra argument except_re that
> defaults to None, and call ExcpectedOutput.matches with this
> argument.
>
> * subversion/tests/cmdline/svnrdump_tests.py
>
> (mismatched_headers_re): Declare a new global string that specifies
> the headers in which a mismatch is expected during a dumping
> operation.
>
> (run_dump_test): Pass the above regular expression string to
> svntest.verify.compare_and_display_lines).
>
> (test_list): Mark copy_and_modify_dump as a passing test.
> ]]]
>
> Index: subversion/tests/cmdline/svnrdump_tests.py
> ===================================================================
> --- subversion/tests/cmdline/svnrdump_tests.py (revision 998490)
> +++ subversion/tests/cmdline/svnrdump_tests.py (working copy)
> @@ -41,6 +41,11 @@ XFail = svntest.testcase.XFail
> Item = svntest.wc.StateItem
> Wimp = svntest.testcase.Wimp
>
> +# Mismatched headers during dumping operation
> +mismatched_headers_re = \
> + "Prop-delta: |Text-content-sha1: |Text-copy-source-md5: |" \
> + "Text-copy-source-sha1: |Text-delta-base-sha1: .*"
> +
So:
* ignore sha1 because RA doesn't provide it yet
* ignore Text-copy-source-md5... why? for the same reason?
* ignore Prop-delta... why?
(It would be good to have the answer in a comment, I think.)
> ######################################################################
> # Helper routines
>
> @@ -80,7 +85,8 @@ def run_dump_test(sbox, dumpfile_name):
>
> # Compare the output from stdout
> svntest.verify.compare_and_display_lines(
> - "Dump files", "DUMP", svnadmin_dumpfile, svnrdump_dumpfile)
> + "Dump files", "DUMP", svnadmin_dumpfile, svnrdump_dumpfile,
> + None, mismatched_headers_re)
>
> def run_load_test(sbox, dumpfile_name):
> """Load a dumpfile using 'svnrdump load', dump it with 'svnadmin
> @@ -177,7 +183,7 @@ test_list = [ None,
> revision_0_load,
> skeleton_load,
> copy_and_modify_load,
> - Wimp("Need to fix headers in RA layer", copy_and_modify_dump),
> + copy_and_modify_dump,
> ]
>
> if __name__ == '__main__':
> Index: subversion/tests/cmdline/svntest/verify.py
> ===================================================================
> --- subversion/tests/cmdline/svntest/verify.py (revision 998490)
> +++ subversion/tests/cmdline/svntest/verify.py (working copy)
> @@ -108,7 +108,7 @@ class ExpectedOutput:
> def __cmp__(self, other):
> raise 'badness'
>
> - def matches(self, other):
> + def matches(self, other, except_re=None):
> """Return whether SELF.output matches OTHER (which may be a list
> of newline-terminated lines, or a single string). Either value
> may be None."""
> @@ -126,8 +126,32 @@ class ExpectedOutput:
> if not isinstance(expected, list):
> expected = [expected]
>
> - return self.is_equivalent_list(expected, actual)
> + if except_re:
> + return self.matches_except(expected, actual, except_re)
> + else:
> + return self.is_equivalent_list(expected, actual)
>
Couldn't you redefine one of these two functions (matches_except() and
is_equivalent_list()) such that they share more code?
For example:
def matches_except(self, expected, actual, except_re):
# TODO: possibly use a slightly different error message
# if an exception is thrown
return self.is_equivalent_list(
filter(lambda x: not re.match(except_re, x), expected),
filter(lambda x: not re.match(except_re, x), actual))
... or add some sort of optional argument to is_equivalent_list(), such
that the functionalities of both functions can be achieved by varying
the value of the optional argument.
In second thought: you could make except_re a lambda expression
(taking a single argument, the line) instead of a regex; i.e.,
s/foo('Text-sha1:')/foo(lambda line: 'Text-sha1' not in line)/
?
> + def matches_except(self, expected, actual, except_re):
> + "Return whether EXPECTED and ACTUAL match except for except_re."
> + if not self.is_regex:
> + i_expected = 0
> + i_actual = 0
> + while i_expected < len(expected) and i_actual < len(actual):
> + if re.match(except_re, actual[i_actual]):
> + i_actual += 1
> + elif re.match(except_re, expected[i_expected]):
> + i_expected += 1
> + elif expected[i_expected] == actual[i_actual]:
> + i_expected += 1
> + i_actual += 1
> + else:
> + return False
> + if i_expected == len(expected) and i_actual == len(actual):
> + return True
> + return False
> + else:
> + raise "self.is_regex and except_re are mutually exclusive"
> +
> def is_equivalent_list(self, expected, actual):
> "Return whether EXPECTED and ACTUAL are equivalent."
> if not self.is_regex:
> @@ -308,7 +332,7 @@ def display_lines(message, label, expected, actual
> sys.stdout.write(x)
>
> def compare_and_display_lines(message, label, expected, actual,
> - raisable=None):
> + raisable=None, except_re=None):
> """Compare two sets of output lines, and print them if they differ,
> preceded by MESSAGE iff not None. EXPECTED may be an instance of
> ExpectedOutput (and if not, it is wrapped as such). RAISABLE is an
> @@ -325,7 +349,7 @@ def compare_and_display_lines(message, label, expe
> actual = [actual]
> actual = [line for line in actual if not line.startswith('DBG:')]
>
> - if not expected.matches(actual):
> + if not expected.matches(actual, except_re):
> expected.display_differences(message, label, actual)
> raise raisable
>
