This is an archive of the discontinued LLVM Phabricator instance.

[LIT] Add option to require PRs for XFAIL directives

Authored by beanz on Sep 28 2016, 9:26 AM.



This patch adds a new flag to lit --xfail-requires-pr which causes lit tests to fail if any XFAIL lines do not also reference a PR. To support this functionality this patch changes how XFAIL lines are parsed to allow one entry in the comma separated list to be a PR (starting with the string "PR").

Diff Detail

Event Timeline

beanz updated this revision to Diff 72850.Sep 28 2016, 9:26 AM
beanz retitled this revision from to [LIT] Add option to require PRs for XFAIL directives.
beanz updated this object.
MatzeB added a subscriber: MatzeB.Sep 28 2016, 12:43 PM

This is great idea. Some comments:

  • I'd make it require a full URL like instead of just PRxxxx so it is easy to get the PR (I am a big fan of Cmd+Click on URL on terminals :)
  • Instead of making it a commandline option, I'd rather put the setting into lit.cfg/ so it will be enforced based on which tests we are running.
beanz added a comment.Sep 28 2016, 1:07 PM

@MatzeB, that's a great suggestion. I'll include that in my rework. I also need to add a method of supporting external bug trackers for out-of-tree projects.

beanz updated this revision to Diff 72924.Sep 28 2016, 4:18 PM
  • Made PR pattern match URL
  • Added support for adding extra PR prefixes
  • Added tests and fixed the bugs I found because of them
MatzeB added inline comments.Sep 28 2016, 5:01 PM

You should use test.config.xFailRequirePR, test.config.bugTrackerRegex after adding some default values to That way these values can be specified in lit.cfg and rather than the lit commandline. That will ensure nobody forgets to use those flags and also allows to run test from two testsuites with different bugtrackers in a single lit run.

As a downstream user I would definitely want to put my additional private prefix in a config file. Don't want to have to remember extra command-line options when running tests individually.

beanz abandoned this revision.Sep 30 2016, 2:46 PM

I had not intended this patch to be the actual review thread (hence not including llvm-commits). I'm going to adapt to the feedback here and re-post to llvm-commits since the llvm-dev thread was mostly favorable.