[libcxx][docs] Make test name pattern documentation more obvious #73136

hawkinsw · 2023-11-22T15:14:28Z

As a new contributor, I found it hard to find the documentation for the meaning of the names of different tests and how those names translate to Lit.

This makes that documentation more explicit.

llvmbot · 2023-11-22T15:15:01Z

@llvm/pr-subscribers-libcxx

Author: Will Hawkins (hawkinsw)

Changes

As a new contributor, I found it hard to find the documentation for the meaning of the names of different tests and how those names translate to Lit.

This makes that documentation more explicit.

Full diff: https://github.com/llvm/llvm-project/pull/73136.diff

1 Files Affected:

(modified) libcxx/docs/TestingLibcxx.rst (+42-7)

diff --git a/libcxx/docs/TestingLibcxx.rst b/libcxx/docs/TestingLibcxx.rst
index 44f3904f4e426a0..5783cc6a21922cc 100644
--- a/libcxx/docs/TestingLibcxx.rst
+++ b/libcxx/docs/TestingLibcxx.rst
@@ -330,13 +330,48 @@ additional headers.
           taken to make it work in earlier language versions.
 
 
-Additional reading
-------------------
-
-The function ``CxxStandardLibraryTest`` in the file
-``libcxx/utils/libcxx/test/format.py`` has documentation about writing test. It
-explains the difference between the test named  ``foo.pass.cpp`` and named
-``foo.verify.cpp`` are.
+Test names
+----------
+
+The names of test files have meaning for the libcxx-specific configuration of 
+Lit. A complete description can be found in the docstring for the function 
+``CxxStandardLibraryTest`` in the file ``libcxx/utils/libcxx/test/format.py``.
+
+For quick reference, here is an overview of some of the most useful patterns:
+
+.. list-table:: Lit Meaning of libcxx Test Filenames
+   :widths: 25 75
+   :header-rows: 1
+
+   * - Name Pattern
+     - Meaning
+   * - ``FOO.pass.cpp``
+     - Compiles, links and runs successfully
+   * - ``FOO.pass.mm``
+     - Same as ``FOO.pass.cpp``, but for Objective-C++
+
+   * - ``FOO.compile.pass.cpp``
+     - Compiles successfully, link and run not attempted
+   * - ``FOO.compile.pass.mm``
+     - Same as ``FOO.compile.pass.cpp``, but for Objective-C++
+   * - ``FOO.compile.fail.cpp``
+     - Does not compile successfully
+
+   * - ``FOO.link.pass.cpp``
+     - Compiles and links successfully, run not attempted
+   * - ``FOO.link.pass.mm``
+     - Same as ``FOO.link.pass.cpp``, but for Objective-C++
+   * - ``FOO.link.fail.cpp``
+     - Compiles successfully, but fails to link
+
+   * - ``FOO.sh.<anything>``
+     - A builtin Lit Shell test
+
+   * - ``FOO.gen.<anything>``
+     - A ``.sh`` test that generates one or more Lit tests on the fly. Executing this test must generate one or more files as expected by LLVM split-file, and each generated file leads to a separate Lit test that runs that file as defined by the test format. This can be used to generate multiple Lit tests from a single source file, which is useful for testing repetitive properties in the library.  Be careful not to abuse this since this is not a replacement for usual code reuse techniques.
+
+   * - ``FOO.verify.cpp``
+     - Compiles with clang-verify. This type of test is automatically marked as UNSUPPORTED if the compiler does not support clang-verify.
 
 Benchmarks
 ==========

libcxx/docs/TestingLibcxx.rst

hawkinsw · 2023-11-22T17:29:51Z

Great! Thank you for the "go ahead". I will send along some fixups and re-request review when it is ready!

hawkinsw · 2023-11-23T02:24:15Z

I have updated to consolidate all documentation about the file name pattern semantics to docs/TestingLibcxx.rst. In the future, I would like to expand the testing documentation to include information about how to write a builtin Lit shell test (including the valid commands, how to use pipelines, valid substitutions, etc). @ldionne : If you would prefer to wait for that additional documentation before merging this PR, that is great by me. If not, I will make a separate PR.

mordante

Thanks for working on this, I have grepped for this information a number times. Having it on the website gives it a better visibility.

LGTM modulo some comments. Please wait for @ldionne to review this too.

mordante · 2023-11-23T20:15:14Z

libcxx/docs/TestingLibcxx.rst

+
+The names of test files have meaning for the libcxx-specific configuration of 
+Lit. A complete description can be found in the docstring for the function 
+``CxxStandardLibraryTest`` in the file ``libcxx/utils/libcxx/test/format.py``.


The last sentence seems no longer to be true.

Good point! :-)

Sorry! I had stupidly not committed a change that fixed this!! Should be resolved now!

mordante · 2023-11-23T20:16:48Z

libcxx/utils/libcxx/test/format.py

-
+    Lit tests are contained in files that follow a certain pattern. That
+    pattern determines the semantics of the test. See
+    libcxx/docs/TestingLibcxx.rst or https://libcxx.llvm.org/TestingLibcxx.html


I would only link to the website and use an anchor.

Good point! Will do!

All done! Great feedback, thank you!

hawkinsw · 2023-11-24T22:01:36Z

Thanks for working on this, I have grepped for this information a number times. Having it on the website gives it a better visibility.

LGTM modulo some comments. Please wait for @ldionne to review this too.

Thank you for the feedback! I really appreciate it!

ldionne

LGTM w/ comments. It would be nice to move the rest of the documentation for the test format to .rst too, otherwise we'll have things in multiple places which means more likely to forget to update new information everywhere.

libcxx/docs/TestingLibcxx.rst

libcxx/utils/libcxx/test/format.py

ldionne · 2023-12-12T15:01:33Z

libcxx/utils/libcxx/test/format.py

+    Lit tests are contained in files that follow a certain pattern. That
+    pattern determines the semantics of the test. See
+    https://libcxx.llvm.org/TestingLibcxx.html#test-names
+    for a complete description of those semantics.

    Substitution requirements


As a follow-up, it would be great to also move this part of the documentation to .rst so that all of the documentation for the file format is in the same place.

hawkinsw · 2023-12-13T00:24:28Z

LGTM w/ comments. It would be nice to move the rest of the documentation for the test format to .rst too, otherwise we'll have things in multiple places which means more likely to forget to update new information everywhere.

Thank you for the review, @ldionne !! I will incorporate your feedback ASAP! The other thing that I would like to add with this PR is documentation on the actual commands available in the lit built-in shell tests. Does that seem reasonable? If so, I will start working on that and try to have something ready for tomorrow!

Thank you again!

ldionne · 2023-12-13T18:06:11Z

The other thing that I would like to add with this PR is documentation on the actual commands available in the lit built-in shell tests.

Which commands do you mean? The actual builtin Lit ShTest commands like RUN? If so, those are already documented in Lit-specific documentation. We should move the documentation for the commands we add in our test format (like ADDITIONAL_COMPILE_FLAGS), though.

hawkinsw · 2023-12-13T19:36:42Z

The other thing that I would like to add with this PR is documentation on the actual commands available in the lit built-in shell tests.

Which commands do you mean? The actual builtin Lit ShTest commands like RUN? If so, those are already documented in Lit-specific documentation. We should move the documentation for the commands we add in our test format (like ADDITIONAL_COMPILE_FLAGS), though.

Great! I will work on that.

I was talking about documenting the builtin lit "shell" commands (e.g.,

    inproc_builtins = {
        "cd": executeBuiltinCd,
        "export": executeBuiltinExport,
        "echo": executeBuiltinEcho,
        "@echo": executeBuiltinEcho,
        "mkdir": executeBuiltinMkdir,
        "popd": executeBuiltinPopd,
        "pushd": executeBuiltinPushd,
        "rm": executeBuiltinRm,
        ":": executeBuiltinColon,
    }

and if/how they differ in their meaning from posix (or PowerShell). Those are not documented in the other lit documentation (e.g., https://llvm.org/docs/TestingGuide.html#writing-new-regression-tests) and it was something that I struggled to find. Would you prefer that I send that as PR directly to lit?

I will reflag you for a review when it is ready! Thanks again!

hawkinsw · 2023-12-14T04:01:28Z

The other thing that I would like to add with this PR is documentation on the actual commands available in the lit built-in shell tests.

Which commands do you mean? The actual builtin Lit ShTest commands like RUN? If so, those are already documented in Lit-specific documentation. We should move the documentation for the commands we add in our test format (like ADDITIONAL_COMPILE_FLAGS), though.

I have added documentation for the ADDITIONAL_COMPILE_FLAGS and FILE_DEPENDENCIES directives.

ldionne · 2023-12-18T15:59:05Z

libcxx/docs/TestingLibcxx.rst

+       *all* the files in any specified directories will be placed in the *current working directory* of the test when it executes. All dependency directories and files
+       specified using relative paths will be anchored to the directory specified by the ``%S`` substitution (i.e., the source directory of the test being executed.).
+   * - ``ADDITIONAL_COMPILE_FLAGS``
+     - ``// ADDITIONAL_COMPILE_FLAGS: flag1, flag2, ...``


This is actually whitespace separated now, so this should be ADDITIONAL_COMPILE_FLAGS: flag1 flag2 .... We forgot to update it, let's do it here.

I believe I got that in the latest fixup.

ldionne · 2023-12-18T15:59:31Z

libcxx/utils/libcxx/test/format.py

-    FOO.verify.cpp          - Compiles with clang-verify. This type of test is
-                              automatically marked as UNSUPPORTED if the compiler
-                              does not support Clang-verify.
-


Additional supported directives below should go away, since it's now in RST.

ldionne · 2023-12-18T16:00:58Z

libcxx/docs/TestingLibcxx.rst

+       specified using relative paths will be anchored to the directory specified by the ``%S`` substitution (i.e., the source directory of the test being executed.).
+   * - ``ADDITIONAL_COMPILE_FLAGS``
+     - ``// ADDITIONAL_COMPILE_FLAGS: flag1, flag2, ...``
+     - The additional compiler flags specified using the ``ADDITIONAL_COMPILE_FLAGS`` libc++-specific Lit directive will be added to the invocation of ``clang`` used to build


This new documentation is not accurate. They don't apply to Clang only, and they do affect .sh.cpp tests too. I suggest we stick to the previous documentation:

This directive will cause the provided flags to be added to the %{compile_flags} substitution for the test that contains it. This allows adding special compilation flags without having to use a .sh.cpp test, which would be more powerful but perhaps overkill.

I believe that I have properly incorporated this feedback as well! Thanks!

ldionne · 2023-12-18T16:02:38Z

libcxx/docs/TestingLibcxx.rst

+     - Usage
+   * - ``FILE_DEPENDENCIES``
+     - ``// FILE_DEPENDENCIES: file, directory, /path/to/file, ...``
+     - The paths given to the ``FILE_DEPENDENCIES`` directive can specify directories or specific files upon which a given test depend. Copies of the files individually specified and


This is slightly incorrect. This uses the %T substitution, not %S. I suggest keeping the wording in the old documentation, I think it wasn't unclear.

The reason I wrote %S was

# For each file dependency in FILE_DEPENDENCIES, inject a command to copy # that file to the execution directory. Execute the copy from %S to allow # relative paths from the test directory.

from line 130 of format.py. Sorry!

Ah, I see -- you're right, this was missing from the old documentation. Maybe something like this?

This directive expresses that the test requires the provided files or directories in order to run. An example is a test that requires some test input stored in a data file. When a test file contains such a directive, the files will collect them and copy them to the directory represented by %T. The copy is performed from the directory represented by %S (i.e. the source directory of the test being executed), which allows using relative paths to specify files. After performing the copy, the intent is that %T contains all the inputs necessary to run the test, such that e.g. execution on a remote host can be done by simply copying %T to the host.

Let me know what you think about what I am about to commit. I think that your suggestion was great -- the examples really help!

hawkinsw · 2023-12-19T02:28:14Z

@ldionne Thank you for the feedback. I am sorry that there were mistakes. I am sure that the mistakes were all my fault but I didn't intend to change any of the documentation that was previously written. I will address your comments as soon as possible. I am going to be away from my keyboard until Thursday but will work on it ASAP.

hawkinsw · 2023-12-23T07:30:06Z

@mordante In re: #76246 I have added some documentation for the MODULE directive. Let me know if it reads okay!

ldionne

LGTM with minor changes to satisfy RST files. Thanks!

ldionne · 2024-01-08T19:12:32Z

libcxx/docs/TestingLibcxx.rst

+     - Same as ``FOO.pass.cpp``, but for Objective-C++.
+
+   * - ``FOO.compile.pass.cpp``
+     - Checks whether the C++ code in the file compiles successfully. In general, prefer `compile` tests over `verify` tests, 


I think you need to use double-backticks here for this to render properly!

I'm surprised this isn't a build error.

I think it's valid RST code, it will just render weird.

Of course I do!! Sorry. Thank you!

As a new contributor, I found it hard to find the documentation for the meaning of the names of different tests and how those names translate to Lit. This makes that documentation more explicit.

Update to consolidate lit test file name pattern semantic documentation to docs/

(Fully) centralize all naming documentation in the rst-based documentation.

@mordante

Point only to www documentation for test names (per @mordante's excellent feedback)

Be more clear about what type of shell we get when running these tests. Co-authored-by: Louis Dionne <[email protected]>

Make it clearer how the tests are generated by libcxx to drive lit according to the patterns embedded in the filenames that contain tests. Co-authored-by: Louis Dionne <[email protected]>

Add documentation about ADDITIONAL_COMPILE_FLAGS and FILE_DEPENDENCIES.

@ldionne

Address @ldionne's astute feedback.

Add link to Internals Manual for writing clang-verify tests.

Add recommendation on when to write verify tests.

Address final comments.

hawkinsw · 2024-01-08T21:55:50Z

Can I get one more check, @ldionne ? Thanks -- I'm just a little nervous!

ldionne

LGTM

…lvm#73136) As a new contributor, I found it hard to find the documentation for the meaning of the names of different tests and how those names translate to Lit. This patch moves the documentation to the RST documentation we publish on the website instead of leaving it in the source code only.

hawkinsw self-assigned this Nov 22, 2023

hawkinsw requested a review from a team as a code owner November 22, 2023 15:14

llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Nov 22, 2023

ldionne requested changes Nov 22, 2023

View reviewed changes

libcxx/docs/TestingLibcxx.rst Show resolved Hide resolved

hawkinsw requested a review from ldionne November 22, 2023 15:20

mordante approved these changes Nov 23, 2023

View reviewed changes

hawkinsw requested a review from mordante November 29, 2023 00:26

ldionne approved these changes Dec 12, 2023

View reviewed changes

hawkinsw requested a review from ldionne December 14, 2023 04:02

ldionne requested changes Dec 18, 2023

View reviewed changes

hawkinsw force-pushed the libcxx_test_docs branch from 88590a4 to eaa17a2 Compare December 21, 2023 20:57

hawkinsw requested a review from ldionne December 21, 2023 20:59

mordante mentioned this pull request Dec 22, 2023

[libc++][modules] Adds module testing. #76246

Merged

hawkinsw force-pushed the libcxx_test_docs branch from dbc75c0 to eaa17a2 Compare December 26, 2023 16:26

ldionne approved these changes Jan 8, 2024

View reviewed changes

hawkinsw and others added 5 commits January 8, 2024 16:36

[libcxx][docs] Make test name pattern documentation more obvious

ec9cd46

As a new contributor, I found it hard to find the documentation for the meaning of the names of different tests and how those names translate to Lit. This makes that documentation more explicit.

fixup! [libcxx][docs] Make test name pattern documentation more obvious

11d5ec3

Update to consolidate lit test file name pattern semantic documentation to docs/

fixup! [libcxx][docs] Make test name pattern documentation more obvious

a3a7740

(Fully) centralize all naming documentation in the rst-based documentation.

fixup! [libcxx][docs] Make test name pattern documentation more obvious

d272cad

Point only to www documentation for test names (per @mordante's excellent feedback)

Update libcxx/docs/TestingLibcxx.rst

29fd8e3

Be more clear about what type of shell we get when running these tests. Co-authored-by: Louis Dionne <[email protected]>

hawkinsw and others added 6 commits January 8, 2024 16:36

Update libcxx/utils/libcxx/test/format.py

b538e94

Make it clearer how the tests are generated by libcxx to drive lit according to the patterns embedded in the filenames that contain tests. Co-authored-by: Louis Dionne <[email protected]>

fixup! [libcxx][docs] Make test name pattern documentation more obvious

7efd1ec

Add documentation about ADDITIONAL_COMPILE_FLAGS and FILE_DEPENDENCIES.

fixup! [libcxx][docs] Make test name pattern documentation more obvious

55b8a07

Address @ldionne's astute feedback.

fixup! [libcxx][docs] Make test name pattern documentation more obvious

be73e31

Add link to Internals Manual for writing clang-verify tests.

fixup! [libcxx][docs] Make test name pattern documentation more obvious

a5f8fbd

Add recommendation on when to write verify tests.

fixup! [libcxx][docs] Make test name pattern documentation more obvious

4f0e726

Address final comments.

hawkinsw force-pushed the libcxx_test_docs branch from b044d25 to 4f0e726 Compare January 8, 2024 21:54

hawkinsw requested a review from ldionne January 8, 2024 21:55

ldionne approved these changes Jan 10, 2024

View reviewed changes

ldionne merged commit d301539 into llvm:main Jan 10, 2024

hawkinsw deleted the libcxx_test_docs branch January 31, 2024 21:58

[libcxx][docs] Make test name pattern documentation more obvious #73136

[libcxx][docs] Make test name pattern documentation more obvious #73136

Conversation

hawkinsw commented Nov 22, 2023

llvmbot commented Nov 22, 2023

hawkinsw commented Nov 22, 2023

hawkinsw commented Nov 23, 2023

mordante left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawkinsw commented Nov 24, 2023

ldionne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawkinsw commented Dec 13, 2023

ldionne commented Dec 13, 2023

hawkinsw commented Dec 13, 2023

hawkinsw commented Dec 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawkinsw commented Dec 19, 2023

hawkinsw commented Dec 23, 2023

ldionne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawkinsw commented Jan 8, 2024

ldionne left a comment

Choose a reason for hiding this comment