Hide cycle in testprojects

Review Request #3600 — Created March 23, 2016 and submitted

stuhood
pants
3058, 3081
pants-reviews
gmalmquist, molsen, peiyu

Cycle detection is tested in test_sort_targets.py, but having a cycle existing directly in the repo breaks implementations of graph parsing that validate cycles early. While testprojects is explicitly for "broken" code, keeping it ./pants listable seems important for maintaining it.

  • Move cycle detection integration test into test_build_graph_integration with mangled BUILD file names.

https://travis-ci.org/pantsbuild/pants/builds/118118736

  • 0
  • 0
  • 2
  • 0
  • 2
Description From Last Updated
  1. A pettern I've used before is to name the BUILD files with broken targets TEST_BUILD, then briefly rename them to BUILD at test-time using a context manager.

    See:

    • testprojects/src/java/org/pantsbuild/testproject/depman/TEST_BUILD,
    • tests/python/pants_test/backend/jvm/subsystems/test_jar_dependency_management_integration.py
    1. Yea, that would work.

      Do you think it is necessary in this case, given that we already have tests covering this? It's not obvious why ivy was testing this.

    2. I don't know; my guess is that Ivy was returning the wrong error at some point, and this was made as a regression test to guard against that.

      Git blame saws that one test-case was touched by dturner, timur, tabishev, zundel, and jsirois. Maybe one of them remembers? :-P

    3. Here's the review where it was first added: https://rbcommons.com/s/twitter/r/1686/

    4. I think it still needs to be an integration test. If you back up to the commit that review is from, test_sort_targets.py was already in place. Which means that this broke, even though test_sort_targets.py was passing.

      Probably what happened is some other (less informative) exception was triggered before the CycleException.

    5. Sorry, I don't remember why cycle1 was referenced in ivy.

  2. 
      
  1. 
      
  2. merge with JarDependencyManagementIntegrationTest._testing_build_file and move to base class PantsRunIntegrationTest so this can be reused for other illegal cases?

  3. 
      
  1. Ship It!
  2. 
      
  1. Nice, thank you!

  2. 
      
Review request changed

Status: Closed (submitted)

Change Summary:

Merged as 5503041efa4c6b9ac892c46da8f0de5715a05322

Loading...