[PATCH] D72358: [llvm-objcopy][test] Fix tests when path contains "bar"

Fangrui Song via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Feb 7 17:17:21 PST 2020


MaskRay added a comment.

In D72358#1864752 <https://reviews.llvm.org/D72358#1864752>, @dblaikie wrote:

> In D72358#1850950 <https://reviews.llvm.org/D72358#1850950>, @MaskRay wrote:
>
> > The argument in D72357 <https://reviews.llvm.org/D72357> does not need to block this patch. I'll push this, as a straightforward improvement over the existing tests, so that @jlebar and @bbaren can be happy.
>
>
> I feel like this kind of ignored specific feedback I had on the other review that applied equally here. Had I known about this review/commit, I would've objected to it on the same grounds.
>
> Please use redirection in this and future tests to address issues like this to help make tests more resilient to interesting paths on developer/test machines.


@dblaikie Almost all llvm-readobj tests do not use input redirection. @rupprecht and @dblaikie raised the concern but @jhenderson and @MaskRay did not think we should do that.

A future `%t` -> `< %t` change will not make this patch entirely meaningless. If @jhenderson and @MaskRay changed their mind, they might create a patch to fix all tests for consistency.
Postponing this patch (2 out of probably 400+ tests) can indeed make the file history a bit more tidy. But does that weigh a lot? What I felt a bit sad was that we left @jlebar and @bbaren's issues unresolved for such a long time. We also missed the release/10.x window. Now a random bar user will find `check-llvm` fail as well.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72358/new/

https://reviews.llvm.org/D72358





More information about the llvm-commits mailing list