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

[Martin Pool]

On 4/20/07, John Arbash Meinel <john at arbash-meinel.com> wrote:
> 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.

Yes, doing that at least for testing (by maybe having a global list of
files that are locked?) would be good.

>
> 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.

But test_initialize doesn't return anything, the state is gc'd after
leaving the section I quote there.

> 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.

Yes, having initialize return the object locked makes sense.

Basically all I was saying is that I'd expect tests to be structured as

  state = Dirstate.initialize()
  try:
     do stuff
  finally:
     state.unlock()

though check_state_with_reopen complicates this because it wants to
close and reopen the state itself.

> > 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.

No, we don't know it's locked.  Depending on when the error occurred
inside check_state_with_reopen or earlier the state may be either
locked or not locked.

Looking at this again I think it would be more correct to split
check_state_with_reopen into two parts, one that looks at the dirstate
within the initial lock context, and another that reopens it, then

  Dirstate.initialize()
  try:
    stuff
    check_open_state
  finally:
    unlock()
  reopen_and_check()

That said, this is not what's breaking win32 so it's not urgent.

-- 
Martin