Add a test for the "buildroot is below the gitroot" case #6339
Conversation
| with temporary_dir(root_dir=get_buildroot()) as worktree: | ||
| with safe_open(os.path.join(worktree, 'README'), 'w') as fp: | ||
| fp.write('Just a test tree.') | ||
| rel_buildroot = fast_relpath(worktree, get_buildroot()) |
There was a problem hiding this comment.
AFAICT you're not usingrel_buildroot anywhere?
| from pants_test.pants_run_integration_test import PantsRunIntegrationTest | ||
| from pants_test.testutils.git_util import initialize_repo | ||
|
|
||
|
|
||
| class ChangedTargetGoalsIntegrationTest(PantsRunIntegrationTest): | ||
| @contextmanager | ||
| def known_commits(self): | ||
| """Creates an anonymous git repository under the buildroot.""" |
There was a problem hiding this comment.
Why is it important for it to be under the real buildroot? I'm getting quite confused by the nesting of real buildroot, test gitroot and test buildroot.
| if os.path.exists(gitdir): | ||
| raise Exception('`known_commits_in` should not be used with an existing git repository.') | ||
|
|
||
| buildroot = os.path.join(worktree, prefix[0]) if prefix else get_buildroot() |
There was a problem hiding this comment.
This is confusing to me. I think "worktree" is the same thing as "gitroot" in the change description? If so it would be good to standardize on a single term.
More confusingly, if I'm grokking correctly, if prefix is specified then the test buildroot is under worktree (which is under the real buildroot), otherwise the test buildroot is the real buildroot, which means it contains worktree?
I feel like I must be reading this wrong.
| def test_list_changed_with_buildroot_ne_gitroot(self): | ||
| # Create a temporary directory, create a mock buildroot in a subdirectory, and then | ||
| # initialize a git repository _above_ the buildroot in the original temp directory. | ||
| with temporary_dir(root_dir=get_buildroot()) as worktree, \ |
There was a problem hiding this comment.
Again, why does this need to be under the real buildroot?
|
The test is still useful, but going to hold onto the branch and close this until I have an abundance of time. |
Problem
#6301 was fixed in #6331, but adding a test to cover it was non-trivial, and so was not added there.
Solution
Add a test to cover #6331.
Result
Fixes #6301. Thanks to @yaup, et al. for the report and original fix!