Make pytest report sources paths relative to the buildroot.

[benjyw]

Previously the new PytestRun task reported paths relative to
the chroot, which was less readable for end users.

Note that this required writing a root conftest.py into the
chroot. Previously we wrote one (for reporting on sharding)
into its own module in a tmpdir, but now we require the
conftest.py hooks to be run when loading each file from the
chroot, which means it has to be in that chroot.

This makes things a little more complicated if the code
happens to have a root-level conftest.py already. We take
some care to temporarily append to this conftest, and then
restore it to its original state. We also take care to be
resilient to failures that somehow prevent the cleanup from
happening.

[stuhood]

Thanks Benjy.

[benjyw]

Did you delete this? I can't find it on the PR (In fact, it looks like you're not listed as a reviewer any more for some reason).

…

On Mon, Apr 17, 2017 at 10:40 AM, Stu Hood ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/python/pants/backend/python/tasks2/pytest_run.py <#4472 (comment)>: > + """Creates a conftest.py to customize our pytest run.""" + + conftest_content = self._get_conftest_content(sources_map) + # We must create our conftest.py in the root of the source chroot, so that its hooks + # run while loading the test files in that chroot. We take care not to stomp on an + # existing conftest.py from the source code. We temporarily append to it instead. + conftest = os.path.join( + self.context.products.get_data(GatherSources.PYTHON_SOURCES).path(), 'conftest.py') + conftest_orig = None + # If there's an existing conftest.py, overwrite it to ensure it contains exactly + # any original user content, plus our temporary additions. + if os.path.exists(conftest): + # First, ensure that we've saved the original conftest.py. + conftest_orig = '{}.orig'.format(conftest) + if not os.path.exists(conftest_orig): + # We only copy if the orig file isn't present, to ensure that the conftest.py we're This comment doesn't seem accurate... it could definitely be the one from a previous failed run. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#4472 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAfS_F18Qrt3ACvRvCyWYl7JnlQzK-nuks5rw6P3gaJpZM4M-wsW> .

[stuhood]

@benjyw : Yea, deleted it.

[benjyw]

So you concur that it's safe? Wondering if I should document better.

…

On Mon, Apr 17, 2017 at 10:44 AM, Stu Hood ***@***.***> wrote: @benjyw <https://github.com/benjyw> : Yea, deleted it. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4472 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAfS_EMzQkNLTL-aaJOtmtS8ellIBRfPks5rw6UUgaJpZM4M-wsW> .

[stuhood]

Yes, I believe that it is safe... was missing the fact that the shutils.copy will overwrite the destination. Only improvement i can think of would be to put the restoration of the old file in a finally block... but then there is a question of debuggability.

[benjyw]

It is, effectively, in a finally block, since this is a contextmanager. I'm changing the comments to better explain why this is safe.

…

On Mon, Apr 17, 2017 at 10:51 AM, Stu Hood ***@***.***> wrote: Yes, I believe that it is safe... was missing the fact that the shutils.copy will overwrite the destination. Only improvement i can think of would be to put the restoration of the old file in a finally block... but then there is a question of debuggability. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4472 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAfS_GxST4BxHa-YzTsxXL-Ckte5yoGVks5rw6awgaJpZM4M-wsW> .

[benjyw]

Retracting "It is, effectively, in a finally block, since this is a contextmanager." The contextmanager still needs a finally block.