Fix nasty BuildGraph bug

Review Request #1337 — Created Nov. 14, 2014 and submitted

patricklaw
pants
02d82b4...
pants-reviews
benjyw, ity, jsirois, zundel

Fixes a nasty bug when injecting target closures in BuildGraph.

Just because a target has been injected doesn't mean that its closure (and therefore its dependency edges) have been injected. The current code misses the case where a target was injected (but not its closure) and then has its closure injected later.

Added a test that identifies the bug, which now passes (doesn't on master): tests/python/pants_test/graph test_inject_then_inject_closure.

CI is green: https://travis-ci.org/pantsbuild/pants/builds/41270498

NH
  1. Could you write a regression test for this? It sure sounds nasty, and I'd like to be sure that the behavior isn't inadvertently reintroduced.

    1. I'll write up a quick unit test.

    2. Added a unit test that fails as expected on master but succeeds after the fix:

                           ============== test session starts ===============
                           platform darwin -- Python 2.7.6 -- py-1.4.26 -- pytest-2.6.4
                           plugins: cov, timeout
                           collected 15 items
      
                           tests/python/pants_test/graph/test_build_graph.py ..F....x..x.xx.
      
                           ==================== FAILURES ====================
                           _ BuildGraphTest.test_inject_then_inject_closure _
      
                           self = <pants_test.graph.test_build_graph.BuildGraphTest testMethod=test_inject_then_inject_closure>
      
                               def test_inject_then_inject_closure(self):
                                 self.add_to_build_file('BUILD',
                                                        'target(name="a", '
                                                        '  dependencies=['
                                                        '    "other:b",'
                                                        '])')
                                 self.add_to_build_file('other/BUILD',
                                                        'target(name="b")')
                                 self.build_graph.inject_address(SyntheticAddress.parse('//:a'))
                                 self.build_graph.inject_address_closure(SyntheticAddress.parse('//:a'))
                                 a = self.build_graph.get_target_from_spec('//:a')
                                 b = self.build_graph.get_target_from_spec('//other:b')
                           >     self.assertIn(b, a.dependencies)
                           E     AssertionError: Dependencies(BuildFileAddress(/private/var/folders/6s/6jff3gfj77b5fm89_ffssm2w0000gp/T/tmpMbcDiK_BUILD_ROOT/other/BUILD, b)) not found in []
      
                           tests/python/pants_test/graph/test_build_graph.py:314: AssertionError
                           = 1 failed, 10 passed, 4 xfailed in 0.41 seconds =
      
      FAILURE
      
      
                     FAILURE
      
    3. Thanks for adding that.

  2. 
      
ZU
  1. Ship It!

  2. 
      
PA
IT
  1. Ship It!

  2. 
      
PA
ZU
  1. Ship It!

  2. 
      
PA
NH
  1. Thanks for the tests!

  2. 
      
PA
Review request changed

Status: Closed (submitted)

Change Summary:

Thanks guys, upstream at bdeb6958135db3adcc19623a0bf7461363fbc73c
Loading...