[PATCH] D95246: [SystemZ][z/OS] Fix No such file or directory expression error matching in lit tests - continued

Mon Jan 25 05:59:31 PST 2021

jhenderson added a comment.

In D95246#2519642 <https://reviews.llvm.org/D95246#2519642>, @abhina.sreeskantharajan wrote:

> In D95246#2518989 <https://reviews.llvm.org/D95246#2518989>, @jhenderson wrote:
>
>> Sorry, could you revert this please. I don't think this is a good fix, as you've reduced coverage within the test, and some changes are completly redundant/add extra noise. I've commented inline with examples. Skimming D94239 <https://reviews.llvm.org/D94239> suggests that has the same issue.
>>
>> Could you also please explain the messages your system is actually producing so we can provide a better solution than this fix.
>>
>> I'm also concerned that you are doing this to fix this test for a system, yet there are no build bots that report this error. A future contributor is likely to break them in the same way/add new tests with the same issue. If your system is a system that is supposed to be supported by LLVM, there needs to be a build bot. If it isn't supported, you should bring this up on llvm-dev (if you haven't already) to get buy-in for support.
>
> Thanks for the feedback. I've reverted my changes from these two patches. We have indicated that we wish to add support for the z/OS platform but we have not set up a buildbot yet.
>
> In D95246#2519086 <https://reviews.llvm.org/D95246#2519086>, @grimar wrote:
>
>> As far I understand, looking on the description of D94239 <https://reviews.llvm.org/D94239>, the message on z/OS looks like "EDC5129I No such file or directory.".
>> I guess the `EDC5129I` is a stable error code? So why not to check for a possible `EDC5129I` prefix word instead of `.*`?
>> (The same applies for other possible errors)
>
> As grimar noted, this is indeed the correct error message.  "EDC5129I No such file or directory." (Note the extra period at the end)
> Based on your feedback, these are the better alternatives that were suggested:

Slightly off-the-wall idea: I'm assuming you don't control your system in such a way that you can change the error message to omit the error code?

>   '{{.*N|n}}o such file or directory' 
>   {{EDC5129I N|N|n}}o such file or directory'
>
> Some testcases fail because of the extra period at the end. For those testcases, this is a possible alternative.
>
>   {{.*N|n}}o such file or directory{{\.?}}
>
> Please let me know if there are better alternatives I could look into.

I think you can just omit the trailing full stop in those cases. If the test isn't using --match-full-lines, it should be fine. If it is, adding `{{.?}}` seems reasonable.

Having the error code explicitly in the pattern looks like the right solution for now, but a thought on that - it seems like tests will still have the fragility problem for when someone else writes a new test that checks the message due to the error code not being present on most systems. Is the error code different for each system error message (I'm guessing it is)? I wonder if we would be better off adding some sort of lit substitution or similar that expands to the right string for the given host OS, which could in turn be fed to FileCheck. It might look a bit like this in practice:

  # RUN: not do-a-thing %t 2>&1 | FileCheck %s -DMSG=%enoent -DFILE=%t

  # CHECK: error: '[[FILE]]': [[MSG]]

What do you think?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95246