On Tue, Mar 15, 2011 at 9:56 AM, Daniel Shahaf <d...@daniel.shahaf.name> wrote: > Paul Burba wrote on Tue, Mar 15, 2011 at 09:38:31 -0400: >> On Tue, Mar 15, 2011 at 12:24 AM, Daniel Shahaf <d...@daniel.shahaf.name> >> wrote: >> > pbu...@apache.org wrote on Tue, Mar 08, 2011 at 15:46:10 -0000: >> >> +++ subversion/trunk/subversion/tests/cmdline/log_tests.py Tue Mar 8 >> >> 15:46:09 2011 >> >> @@ -1148,9 +1148,9 @@ def check_merge_results(log_chain, expec >> >> >> >> # Check to see if the number and values of the revisions is correct >> >> for log in log_chain: >> >> - if (log['revision'] not in expected_merges >> >> - and (expected_reverse_merges is not None >> >> - and log['revision'] not in expected_reverse_merges)): >> >> + if not ((expected_merges and log['revision'] in expected_merges) >> >> + or (expected_reverse_merges >> >> + and log['revision'] in expected_reverse_merges)): >> > >> > If EXPECTED_MERGES and EXPECTED_REVERSE_MERGES are both None, >> > then the if() would trigger --- and I don't think that's the >> > intention. >> >> Hi Daniel, >> >> It is the intention. If EXPECTED_MERGES and EXPECTED_REVERSE_MERGES >> are both None, then the caller believes that no merged revisions >> (normal or reverse) are present. However, there *is* something in the >> LOG_CHAIN, so there is an error. Admittedly, none of the present >> callers pass EXPECTED_MERGES=None and EXPECTED_REVERSE_MERGES=None, >> but we might have reason to do so in the future. >> >> Paul
Possibly I'm abusing a Python convention, but... > I see; I assumed that passing None means "I don't care about this piece > of information; do not attempt to validate it", That makes sense for a function like svntest/actions.py:run_and_verify_svn2(), where if EXPECTED_STDOUT is None, we don't check stdout. That function's core purpose is to invoke main.run_svn(). The caller may or may not care about confirming the stdout, so EXPECTED_STDOUT is justifiably optional. On the other hand, log_tests.py:check_merge_results() has only one purpose, to check that the expected '(Reverse) Merged via' headers match the actual output. That's it. Why would we ever want to ignore either? All that would enable us to do is write a test that spuriously passes. Paul > and that callers who > believe there are no merged revisions would pass an empty dict/list. > > Thanks for the clarification, > > Daniel >
