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
GM
  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. 
      
ST
PE
  1. 
      
  2. merge with JarDependencyManagementIntegrationTest._testing_build_file and move to base class PantsRunIntegrationTest so this can be reused for other illegal cases?

  3. 
      
NH
ST
PE
  1. Ship It!
  2. 
      
GM
  1. Nice, thank you!

  2. 
      
ST
Review request changed

Status: Closed (submitted)

Change Summary:

Merged as 5503041efa4c6b9ac892c46da8f0de5715a05322

Loading...