Don't have JvmToolTaskTestBase require access to "real" option values.

Review Request #2200 — Created May 11, 2015 and discarded

benjyw
pants
1523
pants-reviews
jsirois, zundel

Don't have JvmToolTaskTestBase require access to "real" option values.

See the added README files for some detail on how this is achieved.

Unit tests pass locally. Full CI pending: https://travis-ci.org/pantsbuild/pants/builds/62120618

ZU
  1. 
      
  2. I didn't know you could store symlinks in a git repo.

  3. Why not just self.context() here?

    1. I explain in the comment immediately preceding this line (although a word got deleted from it, so perhaps it was confusing. Fixed that.)

  4. 
      
JS
  1. 
      
  2. src/python/pants/base/config.py (Diff revision 1)
     
     
    Silent dropping seems like a bad idea - is this needed for the current fix?
    1. Not all tests create a fake pants.ini any more, so it's necessary to test for this. And in fact I hope in the near future that no tests at all will.

    2. OK - it makes sense for pants tests not to have to create a pants.ini, but it doesn't make sense for production uses to have their real typoed pants.ini silently ignored.
      IE: if I patch this in on master and run the following I silently just don't get the isolated compile behavior:
      `$ pants.dev --config-override=pants.ini.isolated2 clean-all compile src/java/::`
      
      So I won't block, but I think this is wrong behavior for production code.  It would be nice to find a better way - even if that's just guarding the code path I used above to check for existence and fail before calling load.
    3. Don't we aspire to be able to run pants without a pants.ini anyway? This is certainly possible in the era of all options and no direct config reads, and we are very, very close to that.

    4. Oh I see, but we can check for existence in the explicit --config-override case, yeah, that makes sense.

    5. But also we currently rely on an empty pants.ini to find the build root.  So the aspiration has been ~ `pip install pants && touch pants.ini` and you're up and running.
    6. Good point. So maybe my statement that tests shouldn't need to create an empty pants.ini is actually false.

    7. FYI I'm discarding this RB: I had to make some changes to get a test to pass in some corner case, and I was convinced that pants.ini should always exist, even if empty. So the new code is smaller, and will go out in a new RB soon.

  3. 
      
JS
  1. Ship It!
  2. 
      
BE
BE
Review request changed

Status: Discarded

Loading...