PACK_AFTER_EVERY_COMMIT and svnadmin_tests.py

[Ben Reser <be...@reser.org>]

Using this compile time definition causes two tests to fail in svnadmin_tests.py

{{{
[[[
W: Unexpected output
W: EXPECTED STDOUT:
W: | Packing revisions in shard 1...done.
W: ACTUAL STDOUT:
W: DIFF STDOUT:
W: | --- EXPECTED STDOUT
W: | +++ ACTUAL STDOUT
W: | @@ -1 +0,0 @@
W: | -Packing revisions in shard 1...done.
W: CWD: /Users/breser/wandisco/builds/svn-trunk/subversion/tests/cmdline
W: EXCEPTION: SVNLineUnequal
Traceback (most recent call last):
  File
"/Users/breser/wandisco/share/wcs/svn-trunk/subversion/tests/cmdline/svntest/main.py",
line 1598, in run
    rc = self.pred.run(sandbox)
  File
"/Users/breser/wandisco/share/wcs/svn-trunk/subversion/tests/cmdline/svntest/testcase.py",
line 254, in run
    return self._delegate.run(sandbox)
  File
"/Users/breser/wandisco/share/wcs/svn-trunk/subversion/tests/cmdline/svntest/testcase.py",
line 254, in run
    return self._delegate.run(sandbox)
  File
"/Users/breser/wandisco/share/wcs/svn-trunk/subversion/tests/cmdline/svntest/testcase.py",
line 176, in run
    return self.func(sandbox)
  File
"/Users/breser/wandisco/share/wcs/svn-trunk/subversion/tests/cmdline/svnadmin_tests.py",
line 1800, in hotcopy_incremental_packed
    None, expected_output, [], "pack", os.path.join(cwd, sbox.repo_dir))
  File
"/Users/breser/wandisco/share/wcs/svn-trunk/subversion/tests/cmdline/svntest/actions.py",
line 230, in run_and_verify_svnadmin
    expected_exit, *varargs)
  File
"/Users/breser/wandisco/share/wcs/svn-trunk/subversion/tests/cmdline/svntest/actions.py",
line 238, in run_and_verify_svnadmin2
    expected_stdout, expected_stderr)
  File
"/Users/breser/wandisco/share/wcs/svn-trunk/subversion/tests/cmdline/svntest/verify.py",
line 452, in verify_outputs
    compare_and_display_lines(message, label, expected, actual, raisable)
  File
"/Users/breser/wandisco/share/wcs/svn-trunk/subversion/tests/cmdline/svntest/verify.py",
line 425, in compare_and_display_lines
    raise raisable
SVNLineUnequal
FAIL:  svnadmin_tests.py 28: 'svnadmin hotcopy --incremental' with packing
]]]

[[[
W: Unexpected output
W: EXPECTED STDOUT:
W: | Packing revisions in shard 0...done.
W: | Packing revisions in shard 1...done.
W: | Packing revisions in shard 2...done.
W: ACTUAL STDOUT:
W: DIFF STDOUT:
W: | --- EXPECTED STDOUT
W: | +++ ACTUAL STDOUT
W: | @@ -1,3 +0,0 @@
W: | -Packing revisions in shard 0...done.
W: | -Packing revisions in shard 1...done.
W: | -Packing revisions in shard 2...done.
W: CWD: /Users/breser/wandisco/builds/svn-trunk/subversion/tests/cmdline
W: EXCEPTION: SVNLineUnequal
Traceback (most recent call last):
  File
"/Users/breser/wandisco/share/wcs/svn-trunk/subversion/tests/cmdline/svntest/main.py",
line 1598, in run
    rc = self.pred.run(sandbox)
  File
"/Users/breser/wandisco/share/wcs/svn-trunk/subversion/tests/cmdline/svntest/testcase.py",
line 254, in run
    return self._delegate.run(sandbox)
  File
"/Users/breser/wandisco/share/wcs/svn-trunk/subversion/tests/cmdline/svntest/testcase.py",
line 176, in run
    return self.func(sandbox)
  File
"/Users/breser/wandisco/share/wcs/svn-trunk/subversion/tests/cmdline/svnadmin_tests.py",
line 2375, in verify_packed
    "pack", sbox.repo_dir)
  File
"/Users/breser/wandisco/share/wcs/svn-trunk/subversion/tests/cmdline/svntest/actions.py",
line 230, in run_and_verify_svnadmin
    expected_exit, *varargs)
  File
"/Users/breser/wandisco/share/wcs/svn-trunk/subversion/tests/cmdline/svntest/actions.py",
line 238, in run_and_verify_svnadmin2
    expected_stdout, expected_stderr)
  File
"/Users/breser/wandisco/share/wcs/svn-trunk/subversion/tests/cmdline/svntest/verify.py",
line 452, in verify_outputs
    compare_and_display_lines(message, label, expected, actual, raisable)
  File
"/Users/breser/wandisco/share/wcs/svn-trunk/subversion/tests/cmdline/svntest/verify.py",
line 425, in compare_and_display_lines
    raise raisable
SVNLineUnequal
FAIL:  svnadmin_tests.py 39: verify packed with small shards
]]]
}}}

After some discussion on IRC I started trying to fix this by shifting the
PACK_AFTER_EVERY_COMMIT to a fsfs.conf configuration (committed in r1577362).
So that these tests could turn off packing after every commit to allow them to
actually test svnadmin packing.

Right now we have both PACK_AFTER_EVERY_COMMIT and a --fsfs-packing option to
our tests.  The first one inserts a pack operation as part of the transaction
commit in the libsvn_fs library.  The second one adds a post-commit hook that
does the pack.

According to Daniel, there was a bug found by PACK_AFTER_EVERY_COMMIT that the
--fsfs-packing option didn't find.  I honestly don't see how unless the test
had its own post-commit hook, which there is a conflict.  See:
http://svn.apache.org/r875598

My inclination here is to change --fsfs-packing to simply use the new fsfs.conf
option I set.  Change the failing tests to no longer not run with packing and
instead change the conf to disable packing (i.e. allow individual tests to
override things like --fsfs-packing).

The alternative would be to fix the test suite to support multiple bits of code
for a single hook script.  But that seems far more complicated.

Any other opinions here?

Re: PACK_AFTER_EVERY_COMMIT and svnadmin_tests.py

[Daniel Shahaf <d....@daniel.shahaf.name>]

Ben Reser wrote on Thu, Mar 13, 2014 at 16:51:58 -0700:
> Using this compile time definition causes two tests to fail in svnadmin_tests.py
> 
> {{{
...
> }}}
> 
> After some discussion on IRC I started trying to fix this by shifting the
> PACK_AFTER_EVERY_COMMIT to a fsfs.conf configuration (committed in r1577362).
> So that these tests could turn off packing after every commit to allow them to
> actually test svnadmin packing.
> 
> Right now we have both PACK_AFTER_EVERY_COMMIT and a --fsfs-packing option to
> our tests. The first one inserts a pack operation as part of the transaction
> commit in the libsvn_fs library. The second one adds a post-commit hook that
> does the pack.
> 
> According to Daniel, there was a bug found by PACK_AFTER_EVERY_COMMIT that the
> --fsfs-packing option didn't find. I honestly don't see how unless the test
> had its own post-commit hook, which there is a conflict. See:
> http://svn.apache.org/r875598
> 

r875598 just adds PACK_AFTER_EVERY_COMMIT; the bug you refer to is the one
fixed by <http://svn.apache.org/r875597> (aka r35523).  The reason post-commit
hooks didn't find that bug was that the repository in question had only one
commit, and that commit was r1, which sbox.build() created via 'svnadmin load'
--- which bypasses hooks but did trigger the C post-commit packing in
svn_fs_commit_txn().

> My inclination here is to change --fsfs-packing to simply use the new fsfs.conf
> option I set.

Hmm.  My gut feeling is that there might be future bugs which will be caught by
running 'svnadmin pack' from a hook but not by running it directly in C; but I
don't recall if there were such bugs in the past.

In other words, I think the hook-based packing is still useful, despite
the promotion of the C-based packing (from compile-time knob in
libsvn_fs to unadvertised runtime knob in fs_fs.c).

> Change the failing tests to no longer not run with packing and instead change
> the conf to disable packing (i.e. allow individual tests to override things
> like --fsfs-packing).

+1, since those two tests concern packing specifically.

> The alternative would be to fix the test suite to support multiple bits of code
> for a single hook script. But that seems far more complicated.

AFAICT, the only tests that write a post-commit hook are 'hook_test' and
'post_commit_hook_test' in commit_tests.py, so we might be able to tweak those
two callsites and avoid implementing a general "Multiple post-commit callbacks"
feature in the test harness.

(I haven't checked in detail at our options: perhaps we make those tests
explicitly disable fsfs_packing's hook script, or perhaps we skip them when
fsfs_packing is set, etc.)

> Any other opinions here?