migrate changed integration tests to isolated temp git repos and add an environment variable to override buildroot

Review Request #4295 — Created Oct. 11, 2016 and submitted

yujiec
pants
3906, 3946
pants-reviews
kwlzn

Currently, the integration tests in tests/python/pants_test/engine/legacy/test_changed_integration.py rely on mutating the working copy for the purposes of examining real scm diff information to support the changed suite of goals. this means that if there are uncommitted changes, the git diff output will be altered which can subsequently cause the integration tests to fail.

This change does:
1. Each changed integration test creates an isolated git repo and runs inside it. Note: there are 4 tests that do not mutate working copy, thus they are exempted. The isolated git repo is a subdir of buildroot. The current logic will cause git to panic in this case. Thus I set buildroot to the same git dir as well.
2. Modify get_buildroot(). It will first look for an environment variable PANTS_BUILDROOT_OVERRIDE. If set, then its value will be the buildroot path. If not, the usual way of determining buildroot is used. This env var is purely for testing usage.
3. Shuffle targets in pants_test/base/BUILD to follow alphabetical order of target names.

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

  • 0
  • 0
  • 1
  • 2
  • 3
Description From Last Updated
  1. First, thanks so much for fixing this. It's been one of those frustrating things. I've got a few comments.

    I'm not sure that I like the new relative_to semantics. Before, if someone had GIT_WORK_TREE set to a nested directory, changed would fail. This would expose the possible misconfiguration. With this change, if a pants user were to have a nested git repo and have set GIT_WORK_TREE, pants would not complain, but it's behavior would be odd. I noticed test_changed_target_integration.py also uses this pattern, but it doesn't need the relative_to to change. Maybe there's something to investigate there?

    If there's not a good way to get around the change, I think we should update scm's docstring to note the new behavior, since it may not be expected.

    1. In test_changed_target_integration.py, tests have "--diffspec" argument, which will use "git diff tree" which does not take a "path" argument.
      In test_changed_integration.py, tests eventually calls "git diff -- PATH", where PATH is relative_to. If relative_to is outside of worktree, which is the case here, git diff will error out.

      But I agree this doesn't feel optimal. Let me see if there is another way.

    2. So I came up with another approach of overriding buildroot.

  2. src/python/pants/scm/git.py (Diff revision 1)
     
     

    The other case here is that relative_to is self._worktree, because relative_to was None

    1. This logic is gone in latest revision :)

  3. src/python/pants/scm/git.py (Diff revision 1)
     
     

    I think these would be clearer if they were assigns of list literals. The appends feel like they imply there may be other stateful changes.

    if _: 
      rel_suffix = ['--', self._worktree]
    else:
      rel_suffix = ['--', relative_to]
    

    Is easier to read than

    rel_suffix = ['--']
    if _:
      rel_suffix.append(...)
    else:
      rel_suffix.append(...)
    
  4. src/python/pants/scm/git.py (Diff revision 1)
     
     
     

    Could you add test cases for these cases?

    There is an assertion for the non-existent case, but not for the existent case or a descendant case.

    1. Drop it since the logic is gone.

  5. 
      
  1. thanks for improving this! handful of thoughts.

  2. src/python/pants/scm/git.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     

    seems to me like your new logic here should at least respect - or ideally supplant - the relative_to default assignment two lines above. as-is, it's possible for relative_to to be None, which would then default to self._worktree and inherently pass the self._worktree.startswith(self._worktree) check.

    maybe something along the lines of:

    if relative_to is None or self._worktree.startswith(relative_to):
      rel_suffix = ['--', self._worktree)
    else:
      rel_suffix = ['--', relative_to]
    

    tho it's also unclear why we wouldn't directly respect relative_to if explicitly passed?

    1. relative_to being None is OK. I just want to capture the case where relative_to is ancester of worktree.

      In version 2 I have used fast_relpath instead of startswith.

      The reason of adding new logic is if relative_to is above worktree in dir layout, "git diff -- relative_to" will fail.

      As Nick's comment suggests, I am looking for a new way of solving this.

    2. Drop this since I have come up with another approach for this. This logic is gone :)

  3. src/python/pants/scm/git.py (Diff revision 1)
     
     
     
     
     
     
     

    s/ancester/ancestor/

  4. since you're copying this all over in one fell swoop anyway, I wonder if it might be simpler to recursively copy all of testprojects into the isolated git repo and target that verbatim?

    then you wouldn't need to manage all of the bespoke synthetic path/project creation in addition to the TEST_MAPPING dict.

    1. For every test with isolated repo, we copy the entire testprojects dir to repo?
      I have thought about this, but felt it maybe an overkill.
      But if you think it's fine, then I am happy to do so.

    2. OK so I have tried to copy the entire testprojects dir. With some decent amount of tweaking, I get it to pass at least 1 test (It's easy to make others pass since I know what to do now).
      However, I think it introduces too much dependencies, which makes isolated git repo not so "isolated".

      First, 3rdparty dir has to be copied. Then I have to add docgen backend to recognize "page" symbol, repositories backend to recognize "testing" symbol, and also pythonpath for internal_backend package. In addition, some targets in testprojects have dependency on examples dir, which means I have to copy examples dir over to islated git repo as well.

      I think this adds too much overhead to the tests. So right now I tend not to copy the entire testprojects.
      What do you think?

    3. sure - makes sense.

  5. tests/python/pants_test/scm/test_git.py (Diff revision 1)
     
     

    s/ancester/ancestor/

  6. 
      
  1. Ship it!

    Could you update the summary to note that this introduces a buildroot override env var?

  2. src/python/pants/base/build_root.py (Diff revision 4)
     
     

    Please add a test to test_build_root.py that covers this.

  3. 
      
Review request changed

Status: Closed (submitted)

Change Summary:

Merged as https://github.com/pantsbuild/pants/commit/8fe247971f0361570f7640bfdd732e512f1d3e77.
Thanks Nick and Kris!

Loading...