[rfc] known failure for all tests that fails because of OS locks @ win32

John Arbash Meinel john at arbash-meinel.com
Thu Apr 19 18:19:35 BST 2007


Martin Pool wrote:
> If we don't fix them properly then marking them as known failures
> before the release would be good.

Ultimately I wanted to make OS Locks exclusive on Linux, so I was hoping
to have tests updated to not fail, since they should fail on other
platforms as well.

Obviously I didn't have enough time to fix all of them, though. So
short-term knownFailure is ok.

> 
>> Most of them used pattern try to create reference WT with read lock
>> after they created testing WT with write lock. Because on win32
>> such locks is mutually exclusive I'm give up to fix them.
>>
>> May I mark all this tests as known failure @ win32?
> 
> I had a look at TestDirStateInitialize, which seemed like the simplest one:
> 
> 639 class TestDirStateInitialize(TestCaseWithDirState):
> 640
> 641     def test_initialize(self):
> 642         expected_result = ([], [
> 643             (('', '', 'TREE_ROOT'), # common details
> 644              [('d', '', 0, False, dirstate.DirState.NULLSTAT), #
> current tree
> 645              ])
> 646             ])
> 647         state = dirstate.DirState.initialize('dirstate')
> 648         try:
> 649             self.assertIsInstance(state, dirstate.DirState)
> 650             lines = state.get_lines()
> 651             self.assertFileEqual(''.join(state.get_lines()),
> 652                 'dirstate')
> 653             self.check_state_with_reopen(expected_result, state)
> 654         except:
> 655             state.unlock()
> 656             raise
> 
> First, I don't see why we're unlocking from an except block not a
> finally block.  The test always ought to do that to clean up.

I think it is because we want to return a Write-locked state if
everything succeeds.

IIRC it is because the tests are cleaner/easier to write if the locking
is taken care of with "state.lock()" and "self.addCleanup(state.unlock)"

The reason for 'initialize' to leave it locked is because you almost
always need it that way, and it needs it locked to initialize. So adding
an extra unlock just to have callers always lock seemed foolish.

> 
> It seems a bit strange that the check_state_with_reopen is inside this
> block that holds the lock.  check_state_with_unlock releases the lock
> as a side effect, which makes it hard to get the calling code's
> cleanup right.  I suppose that wouldn't normally matter in tests where
> we don't care about handling unexpected errors but it might be
> complicating things here.

The
try/except/raise

Is because we know the 'state' is locked, and we don't want to
complicate the test failure by having the "Lock not released"
warning/error cluttering things up.

It would be possible to always unlock, and then lock before we return.

And certainly if we are doing a read lock during a write lock, we should
do that anyway.
John
=:->



More information about the bazaar mailing list