This is an archive of the discontinued LLVM Phabricator instance.

[PATCH] [llvm] [lit] Fix inconsistent test order in shtest-keyword-parse-errors
ClosedPublic

Authored by mgorny on Aug 4 2021, 12:01 AM.

Details

Summary

Remove test times when running shtest-keyword-parse-errors test,
in order to prevent the previous executions from impacting subtest
order and therefore causing FileCheck to fail.

Diff Detail

Event Timeline

mgorny requested review of this revision.Aug 4 2021, 12:01 AM
mgorny created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptAug 4 2021, 12:01 AM
jdenny accepted this revision.Aug 4 2021, 1:24 PM
jdenny added a subscriber: lebedev.ri.

Other than a tiny nit, LGTM.

llvm/utils/lit/tests/shtest-keyword-parse-errors.py
2

@lebedev.ri has been marking these lines with a FIXME: ef4b3a4571e2.

Perhaps it's good to do that consistently until someone implements a more comprehensive fix (like putting the cleanup or some command-line option in %{lit}).

This revision is now accepted and ready to land.Aug 4 2021, 1:24 PM
mgorny marked an inline comment as done.Aug 5 2021, 12:17 AM
mgorny added inline comments.
llvm/utils/lit/tests/shtest-keyword-parse-errors.py
2

Will do, thanks!

This revision was automatically updated to reflect the committed changes.
mgorny marked an inline comment as done.
jdenny added a comment.Aug 5 2021, 8:31 AM

Thanks for the fix!

mgorny added a comment.Aug 5 2021, 8:47 AM

You're welcome. I'd love to try to figure out how to make the tests not depend on subtest order but my TODO is too long already.

jdenny added a comment.Aug 5 2021, 9:06 AM

You're welcome. I'd love to try to figure out how to make the tests not depend on subtest order but my TODO is too long already.

Yes, it's not my highest priority either.

I think some sort of command-line option to disable this behavior is the right solution, and it can be embedded in %{lit} so that no test is affected accidentally. (I'd like to use that in clang/test/utils as well.) For tests that specifically check this behavior (reorder.py), an alternate substitution (e.g., %{lit-reordered}) can be provided.

mgorny added a comment.Aug 5 2021, 9:13 AM

Indeed. In fact, the first thing I did was to see --help if there's a way to disable these files. To be honest, I don't really like the idea of lit leaving cache files all over the place.

To be honest, I don't really like the idea of lit leaving cache files all over the place.

I think you're saying that users of lit (as opposed to maintainers of lit's test suite) might like such a command-line option too.

mgorny added a comment.Aug 5 2021, 9:21 AM

Exactly. Or simply a new lexical test order option.

mgorny added a comment.Aug 7 2021, 7:42 AM

Ok, I've decided to spend 10 minutes and implement a lexical test order option: D107695.