[PATCH] D78589: [lit] Add an option to print all features used in tests

[Julian Lettner via Phabricator via llvm-commits]

yln added a comment.

I like this.  Thanks for working on it!

Please check my assumptions, about features:
Available, but unused -> cruft
Not available, but used -> okay (e.g., test disabled because not right platform/architecture)

After adding all my comments above, I am assuming the workflow is as follows:
First run with `--show-suites` to see available features, then run `--show-features` to see used/requested features and cleanup the ones which are available but unused.

How about making this even easier and more discoverable by adding it directly to `--show-suites` to encourage people to clean things up?

Append "(unused)" marker to available, but unused feature to draw attention that they could be removed; or maybe even add a separate line for "unused" features.
Add another line for "unavailable" features.

  Available features: darwin, win95 (unused)
  Unavailable/requested features: linux

or

  Available features: darwin
  Unavailable features: linux
  Unused features: win95

What do you think?



================
Comment at: llvm/utils/lit/lit/Test.py:371
+        parsed = lit.TestRunner._parseKeywords(self.getSourcePath(), require_script=False)
+        unsupported = parsed['UNSUPPORTED:'] or []
+        requires = parsed['REQUIRES:'] or []
----------------
Is parsed a dict? If so, then I think we get a KeyError here; or the `or []` part is redundant.


================
Comment at: llvm/utils/lit/lit/Test.py:373
+        requires = parsed['REQUIRES:'] or []
+        xfails = parsed['XFAIL:'] or []
+        tokens = itertools.chain.from_iterable(
----------------
`feature_keywords = ['UNSUPPORTED', ..]`
and then do some mapping over that: keywords -> dict lookup -> chain -> tokenize -> chain


================
Comment at: llvm/utils/lit/lit/Test.py:376
+            BooleanExpression.tokenize(expr) for expr in
+                itertools.chain(unsupported, requires, xfails) if expr != '*'
+        )
----------------
Prefer being dry over performance, remove `if expr != '*'`


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1462
+    parsed = _parseKeywords(test.getSourcePath(), additional_parsers, require_script)
+    script = parsed['RUN:']
+    test.xfails = parsed['XFAIL:'] or []
----------------
KeyError or redundant `or []`?


================
Comment at: llvm/utils/lit/lit/cl_arguments.py:159
             action="store_true")
+    debug_group.add_argument("--show-features",
+            help="Show all features used in the test suite (in XFAIL, UNSUPPORTED and REQUIRES) and exit",
----------------
Maybe name `--show-used-features`?


================
Comment at: llvm/utils/lit/lit/cl_arguments.py:160
+    debug_group.add_argument("--show-features",
+            help="Show all features used in the test suite (in XFAIL, UNSUPPORTED and REQUIRES) and exit",
+            action="store_true")
----------------
Show features used by selected tests; or use discovered_tests below


================
Comment at: llvm/utils/lit/lit/main.py:78
+        print(' '.join(sorted(features)))
+        sys.exit(0)
+
----------------
If the main use case for this is identifying obsolete features that are defined, but never used then we could move this up and use discovered_tests.  Or is it also useful to ask the question, which features do these selected/filtered tests use?


================
Comment at: llvm/utils/lit/lit/main.py:140
     if show_suites:
         import itertools
         tests_by_suite = itertools.groupby(tests, lambda t: t.suite)
----------------
Import at top is better; remove


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78589