Page MenuHomePhabricator

[LIT] Add option to require PRs for XFAIL directives
Needs RevisionPublic

Authored by beanz on Sep 30 2016, 4:10 PM.

Details

Summary

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 "http://llvm.org/PR").

By default this option is globally disabled in lit. It can be globally enabled by passing the flag, and it can be disabled in a test suite's lit.cfg or lit.site.cfg, using the option config.xFailRequirePR. Additionally extra PR prefixes can be specified globally using the --bug-tracker-prefix flag, or per-test suite by setting config.bugTrackerPrefixes.

Event Timeline

beanz updated this revision to Diff 73159.Sep 30 2016, 4:10 PM
beanz retitled this revision from to [LIT] Add option to require PRs for XFAIL directives.
beanz updated this object.
beanz added reviewers: probinson, MatzeB.
beanz added a subscriber: llvm-commits.
ddunbar edited edge metadata.Oct 4 2016, 10:40 AM

Cute, I like the idea.

Is there a reason to have the command line option, versus just requiring this if the prefixes are defined?

Also, on the syntax, wouldn't it be simpler to say this should look like:

XFAIL: condition, condition # PR or description
beanz added a comment.Oct 4 2016, 11:24 AM

I am completely flexible on the syntax. The option os for globally enabling the feature which is disabled by default. There is a RFC thread going on LLVM-dev about whether or not we want to make the accompanying policy change project-wide (pending an audit of existing XFAILs).

modocache requested changes to this revision.EditedOct 6 2016, 8:42 AM
modocache edited edge metadata.

This is a great idea, thanks!

I agree with @ddunbar's suggestion on the syntax.

utils/lit/lit/main.py
276

Looks like this will have to be rebased -- these have since been migrated to argparse and add_argument in rL283152.

I agree with @ddunbar's suggestion: specifying --bug-tracker-prefix should imply --xfail-requires-pr, which would eliminate the need for this option.

nit-pick: Modern Python's naming convention for variables is snake_case (https://www.python.org/dev/peps/pep-0008/#function-names), which would make this variable name xfail_requires_pr. One nice part about argparse is that foo-bar is automatically converted to an attribute named foo_bar, so we could eliminate the dest= parameter here. Still, I guess there's little value in migrating this codebase from mixedCase to snake_case, whether incrementally or all at once. But I was wondering if maybe @ddunbar has any thoughts here?

277

nit-pick: If we use "PR" in the help messages for lit, it'd be great to spell out "problem report" in at least one spot in the help messages. I think there's room for confusion, since GitHub users are used to "PR" meaning "pull request".

Related: D25331

282

You can use %(default)s instead of writing out the default value. argparse will replace it with the value specified in the default= parameter.

This revision now requires changes to proceed.Oct 6 2016, 8:42 AM