Ensure that every test begins with a properly reset build root.

Review Request #2165 — Created May 5, 2015 and discarded

benjyw
pants
1492
pants-reviews
davidt, patricklaw, zundel
This is very important: A failure in any test subclass's setUp()
method would cause the unittest framework not to run tearDown().
And BaseTest strongly assumes that tearDown() always runs in order
to clean up state that must not persist across tests.

Chief among this state is the buildroot. If setUp() crashes after
setting the buildroot to the tmpdir, then tearDown() won't run,
and the next test will see the previous test's tmpdir as its
real buildroot. And because we compute relative paths against that
buildroot, we can in some cases end up with a path relative to the
bad buildroot pointing into the real buildroot! If you've ever seen
files mysteriously deleted after a test failure, and had to re-check
 them out of git, this is why.

Because this is so dangerous, we aggressively crash the entire test
run if we detect this state.

Note that we use the build root setting as a proxy for all the state
that's set up in setUp() and reset in tearDown(), because it's by far
the most important such state.

In the future we hope to get rid of the need to know the real buildroot,
but for now it's vital not to misdetect it.

CI in flight: https://travis-ci.org/pantsbuild/pants/builds/61385432

JS
  1. 
      
  2. tests/python/pants_test/base_test.py (Diff revision 1)
     
     
    How about just adding a cleanup here: https://docs.python.org/2/library/unittest.html#unittest.TestCase.addCleanup
    
    I have not tested but the docs say this does as you'd expect on a raise and unwinds the cleanups so-far registered: https://docs.python.org/2/library/unittest.html#unittest.TestCase.doCleanups
    
    I like this pattern better to boot, I migrated to it using google's TearDownTestCase and addTearDown in java land as well - there just for locality reasons - its nice to see the resource cleanup in the line after the resource establishment.
    1. I did not know about addCleanup, but that should definitely do what we need. I will scrap this change and send out a new one.

      Welcome back!?

  3. 
      
BE
Review request changed

Status: Discarded

Loading...