A few fixes to config path computation, esp. in tests.

Review Request #3709 — Created April 17, 2016 and submitted

benjyw
pants
3195
pants-reviews
mateor, stuhood

This gets us part of the way towards making integration
tests hermetic, and unreliant on our real pants.ini.

With this commit, all unittests pass if we move our pants.ini
to a different location.

This commit also ensures that option and contrib/scrooge's
integration tests run if we move pants.ini.

It will take many followups to get all integration tests
to behave similarly.

CI passes: https://travis-ci.org/pantsbuild/pants/builds/123700526

Moved pants.ini and ran various tests.

  • 0
  • 0
  • 1
  • 0
  • 1
Description From Last Updated
  1. You should add Stu to this review - he strongly felt that the old behavior was correct.

    But I am glad to see it - I spent a bunch of time digging into this before the weekend and it was pretty frustrating.

    1. The old behavior, as in - the integration tests depend on the pants.ini we use to build pants itself? That seems wrong to me.

    2. Yeah - we discussed it in slack on Friday. I already ran my version of this patch through Travis but I closed the PR without submitting after our conversation. The slack convo starts here: https://pantsbuild.slack.com/archives/general/p1460670638003476, probably another good example of something we could have taken to the mailing list.

      My patch was here: https://github.com/pantsbuild/pants/pull/3192/commits/7eb9fd42a33f8d716e95afd1601c47741d71feb4. It was pretty simple - I was still flushing out which other tests implictly relied on the cache being disabled.

      I also ran a test Travis run with the buildcache disabled for all runs (just passing the --no-cache-read flag globally) but saw incredible slow down and the same timeouts you generated.

  2. So I don't think that it is completely hermetic yet. I was able to get a test to fail by applying the following config patch and running PANTS_CONFIG_OVERRIDE="['pants.cache.ini']" ./pants test contrib/scrooge:::

    diff --git a/pants.cache.ini b/pants.cache.ini
    index fffb3b9..47b6cf4 100644
    --- a/pants.cache.ini
    +++ b/pants.cache.ini
    @@ -7,3 +7,7 @@ local_artifact_cache = %(buildroot)s/.buildcache
     [cache]
     read_from = ["%(local_artifact_cache)s"]
     write_to = ["%(local_artifact_cache)s"]
    +
    +
    +[thrift-linter]
    +strict: False
    \ No newline at end of file
    

    That is because the PANTS_CONFIG_OVERRIDE is being passed to the integration test along with the rest of the env. The option system then appends it to the --config-overrides value. But there looks to be an easy fix AFAICT - instead of passing the --config-override flag, you could pass it as an extra_env where it will override any passed PANTS_CONFIG:

    ```
    diff --git a/tests/python/pants_test/pants_run_integration_test.py b/tests/python/pants_test/pants_run_integration_test.py
    index 3142094..ed12841 100644
    --- a/tests/python/pants_test/pants_run_integration_test.py
    +++ b/tests/python/pants_test/pants_run_integration_test.py
    @@ -129,6 +129,7 @@ class PantsRunIntegrationTest(unittest.TestCase):
    if self.hermetic():
    args.extend(['--pants-config-files=[]', '--no-cache-read', '--no-cache-write'])

    • extra_env = extra_env if extra_env else {}
      if config:
      config_data = config.copy()
      ini = ConfigParser.ConfigParser(defaults=config_data.pop('DEFAULT', None))
      @@ -139,7 +140,7 @@ class PantsRunIntegrationTest(unittest.TestCase):
      ini_file_name = os.path.join(workdir, 'pants.ini')
      with safe_open(ini_file_name, mode='w') as fp:
      ini.write(fp)
    • args.append('--config-override=' + ini_file_name)
    • extra_env['PANTS_CONFIG_OVERRIDE'] = ini_file_name

      pants_script = os.path.join(get_buildroot(), self.PANTS_SCRIPT_NAME)
      pants_command = [pants_script] + args + command

    ```

    1. Well I'm not hewing to a completely literal interpretation of the word "hermetic". God knows what else would need to happen to make this truly hermetic.

      But in this case we should scrub the run_pants environment, I think. Any options we set on the cmd line are for the test runner, not for the pants instance that the test happens to run. If you want to set env for that thing, you have to do it in the test code.

    2. Pushing a commit that makes hermetic integration tests ignore env vars.

  3. 
      
  1. Looks great! Thanks Benjy.

  2. 
      
  1. 
      
  2. Note*

    1. Haha, yes, "Not" is the opposite of what I want to say...

  3. As it stands, this will cause tools to be rebootstrapped each time...

    I think that defaults-wise, enabling the cache for: [cache.bootstrap] might be good.

    1. Very good point! Will do that.

  4. 
      
  1. Stu, PTAL and see if this makes sense. Thanks!

  2. 
      
  1. 
      
  2. .gitignore (Diff revision 4)
     
     

    .buildcache is already in here, but I like .cache better... should we swap over the other config to point to .cache, or is this supposed to be isolated for unit tests? If it's supposed to be isolated for unit tests, would be good to name it to indicate that.

    1. I don't think this is supposed to be isolated for tests (note - integration, not unit), this just happens to be the location that is used when there's no pants.ini to override it (the hardcoded default is [os.path.join(get_buildroot(), '.cache')]). We happen to override it in our pants.ini to local_artifact_cache: %(pants_bootstrapdir)s/artifact_cache.

      I don't even know what .buildcache is.

    2. My understanding of that is the .local_artifact_cache or equivalent. The integration tests were already running in a temp workdir, so they never relied on pants.d artifacts. But the original tests did not consider the local artifact cache and so were written to be reliant on it in places.

  3. tests/python/pants_test/pants_run_integration_test.py (Diff revision 4)
     
     
     
     
     
     
     

    This will cause os.environ to overwrite extra_env, rather than the reverse. Intentional?

    1. Oh good point, I wrote that because it was slightly more convenient and I figured the order doesn't matter, but of course it does. Will change.

  4. 
      
  1. 
      
  2. my preference would be to inline this, feel free to ignore if you disagree.

    env = {} if self.hermetic else os.environ.env
    
  3. I am surprised that this works - I would have thought the global flag took precedent.

    Is the implication that it is impossible to turn off cache reads for integration test bootstrapping? Say if I was on bad wifi or something?

    This isn't a blocker for me - just wondering.

    1. No, inner scope options override the values they inherit from outer scopes at the same rank (if we had set the inner scope value in config then the outer scope flag would indeed take precedence).

      Also, this is all local cache, so wifi isn't an issue. It's mostly just to speed up CIs.

  4. 
      
Review request changed

Status: Closed (submitted)

Change Summary:

87d9909350d91ecdfd0bb5ba95d6ce58e6bde219

  1. Submitted. Thanks Mateo and Stu!

  2. 
      
Loading...