[lit] Extend --xfail/LIT_XFAIL to take full test name

[jdenny]

Summary

The new documentation entry gives an example use case from
libomptarget.

[protze.joachim]

Since you refer to libomptarget, I'm wondering how you want to use this.
Do you plan to use this for manual lit invocation only or is the plan to hard-wire this option into check targets?

[jdenny]

In D105208#2850670, @protze.joachim wrote:

Since you refer to libomptarget, I'm wondering how you want to use this.
Do you plan to use this for manual lit invocation only or is the plan to hard-wire this option into check targets?

The former. My use case is my own CI config. That is, there are some upstream tests that fail, and there are people investigating to fix them, but it can take a long time. While I'm waiting, I want my CI to be green so I can easily see problems from patches I'm working on. I use these options in my CI config to achieve that.

This patch just makes it possible to be more precise in specifying tests. I cc'ed you because it's relevant after your D101326 (a nice improvement, BTW).

For background, D96662 has the original --xfail/LIT_XFAIL review.

[yln]

In D105208#2850746, @jdenny wrote:

This patch just makes it possible to be more precise in specifying tests.

My understanding of this change: a test file/declaration can produce multiple test executions (by being included in multiple test configurations). Previously, we could only xfail based on test name (suite-relative file path), which meant "all or none" test executions. This patch allows us to also use the fully-qualified test execution name (which includes the test configuration) to be more selective.

LGTM! Thanks also for the really good documentation! :)

(One thought I had: we could generalize this further by only requiring a substring match instead of an exact match on file_path or full_name; but probably no need for this right now.)

[jdenny]

In D105208#2851016, @yln wrote:

In D105208#2850746, @jdenny wrote:

This patch just makes it possible to be more precise in specifying tests.

My understanding of this change: a test file/declaration can produce multiple test executions (by being included in multiple test configurations).

It's also conceivable that the same file name might happen to appear in unrelated test suites, but I don't recall actually running into that case.

Previously, we could only xfail based on test name (suite-relative file path), which meant "all or none" test executions. This patch allows us to also use the fully-qualified test execution name (which includes the test configuration) to be more selective.

Exactly.

LGTM! Thanks also for the really good documentation! :)

Thanks for the quick review! Before pushing, I'll give @davezarzycki a few days to comment as the --xfail implementation was his.

(One thought I had: we could generalize this further by only requiring a substring match instead of an exact match on file_path or full_name; but probably no need for this right now.)

That's something to think about, but I'm not sure. My concern is additional unintentional collisions: foo.c would match my/foo.c, my/foo.cpp, and your/foo.c. I feel like suppressing test failures ought to be precise, and I think that's why @davezarzycki decided against regexes here.

[jhenderson]

LGTM too.

In D105208#2851144, @jdenny wrote:

In D105208#2851016, @yln wrote:

(One thought I had: we could generalize this further by only requiring a substring match instead of an exact match on file_path or full_name; but probably no need for this right now.)

That's something to think about, but I'm not sure. My concern is additional unintentional collisions: foo.c would match my/foo.c, my/foo.cpp, and your/foo.c. I feel like suppressing test failures ought to be precise, and I think that's why @davezarzycki decided against regexes here.

Yeah, we need to be VERY careful with substring matches, due to the risk of false XFAILs for tests that should be real failures. We don't want real test failures be hidden by accident after all.

[davezarzycki]

LGTM too. And I agree that substring matching would be undesirable.

[jdenny]

Thanks for the reviews.

llvm/docs/CommandGuide/lit.rst

 .. option:: --xfail=LIST
 
   Treat those tests whose name is in the semicolon separated list ``LIST`` as
   ``XFAIL``. This can be helpful when one does not want to modify the test
   suite. The environment variable ``LIT_XFAIL`` can be also used in place of
   this option, which is especially useful in environments where the call to
   ``lit`` is issued indirectly.
 
+  A test name can specified as a file name relative to the test suite directory.
+  For example:
+
+  .. code-block:: none
+
+    LIT_XFAIL="affinity/kmp-hw-subset.c;offloading/memory_manager.cpp"
+
+  In this case, all of the following tests are treated as ``XFAIL``:
+
+  .. code-block:: none
+
+    libomp :: affinity/kmp-hw-subset.c
+    libomptarget :: nvptx64-nvidia-cuda :: offloading/memory_manager.cpp
+    libomptarget :: x86_64-pc-linux-gnu :: offloading/memory_manager.cpp
+
+  Alternatively, a test name can be specified as the full test name
+  reported in LIT output.  For example, we can adjust the previous
+  example not to treat the ``nvptx64-nvidia-cuda`` version of
+  ``offloading/memory_manager.cpp`` as XFAIL:
+
+  .. code-block:: none
+
+    LIT_XFAIL="affinity/kmp-hw-subset.c;libomptarget :: x86_64-pc-linux-gnu :: offloading/memory_manager.cpp"
+
+
 ADDITIONAL OPTIONS
 ------------------
 
 .. option:: --debug
 
  Run :program:`lit` in debug mode, for debugging configuration issues and
  :program:`lit` itself.
 

llvm/utils/lit/lit/main.py

msg = f'Selecting shard {run}/{shards} = ' \
           f'size {len(selected_tests)}/{len(tests)} = ' \
           f'tests #({shards}*k)+{run} = [{preview}]'
     lit_config.note(msg)
     return selected_tests
 
 
 def mark_xfail(selected_tests, opts):
     for t in selected_tests:
-        if os.sep.join(t.path_in_suite) in opts.xfail:
+        test_file = os.sep.join(t.path_in_suite)
+        test_full_name = t.getFullName()
+        if test_file in opts.xfail or test_full_name in opts.xfail:
             t.xfails += '*'
 
 def mark_excluded(discovered_tests, selected_tests):
     excluded_tests = set(discovered_tests) - set(selected_tests)
     result = lit.Test.Result(lit.Test.EXCLUDED)
     for t in excluded_tests:
         t.setResult(result)
 

llvm/utils/lit/tests/Inputs/xfail-cl/a/false.txt

+# RUN: false

llvm/utils/lit/tests/Inputs/xfail-cl/a/lit.cfg

+import lit.formats
+config.name = 'top-level-suite :: a'
+config.suffixes = ['.txt']
+config.test_format = lit.formats.ShTest()

llvm/utils/lit/tests/Inputs/xfail-cl/a/test.txt

+# RUN: true

llvm/utils/lit/tests/Inputs/xfail-cl/b/false.txt

+# RUN: false

llvm/utils/lit/tests/Inputs/xfail-cl/b/lit.cfg

+import lit.formats
+config.name = 'top-level-suite :: b'
+config.suffixes = ['.txt']
+config.test_format = lit.formats.ShTest()

llvm/utils/lit/tests/Inputs/xfail-cl/b/test.txt

+# RUN: false

llvm/utils/lit/tests/xfail-cl.py

-# Check that regex-XFAILing works and can be configured via env var.
-#
-# RUN: %{lit} --xfail 'false.txt;false2.txt' %{inputs}/xfail-cl | FileCheck --check-prefix=CHECK-FILTER %s
-# RUN: env LIT_XFAIL='false.txt;false2.txt' %{lit} %{inputs}/xfail-cl | FileCheck --check-prefix=CHECK-FILTER %s
+# Check that XFAILing works via command line or env var.
+
+# RUN: %{lit} --xfail 'false.txt;false2.txt;top-level-suite :: b :: test.txt' \
+# RUN:        %{inputs}/xfail-cl \
+# RUN: | FileCheck --check-prefix=CHECK-FILTER %s
+
+# RUN: env LIT_XFAIL='false.txt;false2.txt;top-level-suite :: b :: test.txt' \
+# RUN: %{lit} %{inputs}/xfail-cl \
+# RUN: | FileCheck --check-prefix=CHECK-FILTER %s
+
 # END.
-# CHECK-FILTER: Testing: 3 tests, {{[1-3]}} workers
-# CHECK-FILTER-DAG: XFAIL: top-level-suite :: false.txt
-# CHECK-FILTER-DAG: XFAIL: top-level-suite :: false2.txt
-# CHECK-FILTER-DAG: PASS: top-level-suite :: true.txt
+
+# CHECK-FILTER: Testing: 7 tests, {{[1-7]}} workers
+# CHECK-FILTER-DAG: {{^}}PASS: top-level-suite :: a :: test.txt
+# CHECK-FILTER-DAG: {{^}}XFAIL: top-level-suite :: b :: test.txt
+# CHECK-FILTER-DAG: {{^}}XFAIL: top-level-suite :: a :: false.txt
+# CHECK-FILTER-DAG: {{^}}XFAIL: top-level-suite :: b :: false.txt
+# CHECK-FILTER-DAG: {{^}}XFAIL: top-level-suite :: false.txt
+# CHECK-FILTER-DAG: {{^}}XFAIL: top-level-suite :: false2.txt
+# CHECK-FILTER-DAG: {{^}}PASS: top-level-suite :: true.txt