[libcxx-commits] [PATCH] D152008: [libc++] Use .gen.py tests for the transitive inclusion tests

[Mark de Wever via Phabricator via libcxx-commits]

Mordante accepted this revision.
Mordante added a comment.
This revision is now accepted and ready to land.

LGTM modulo one nit.



================
Comment at: libcxx/test/libcxx/transitive_includes.gen.py:27-28
+
+# To re-generate the list of expected headers, temporarily set this to True, re-generate
+# the file and run this test.
+# Note that this needs to be done for all supported language versions of libc++:
----------------
Mordante wrote:
> ldionne wrote:
> > Mordante wrote:
> > > These instructions seem outdated an no longer working. The regeneration happen when I try to run this test. Unfortunately running the test fails.
> > > 
> > > `<build>/bin/llvm-lit --param std=c++03 test/libcxx/transitive_includes.gen.py` gives
> > > ```
> > > <llvm>/llvm/utils/lit/lit/discovery.py:189: error: 'llvm-libc++-shared.cfg.in :: libcxx/transitive_includes.gen.py' would not be run indirectly: change name or LIT config(e.g. suffixes or standalone_tests variables)
> > > 1 errors, exiting
> > > ```
> > > 
> > > Running the generated test manually 
> > > `<build>/bin/llvm-lit <build>/build/test/libcxx/transitive_includes.gen.py/algorithm.sh.cpp`
> > > gives
> > > ```
> > > 
> > > llvm-lit: <llvm>/llvm/utils/lit/lit/discovery.py:137: warning: unable to find test suite for '<build>/test/libcxx/transitive_includes.gen.py/algorithm.sh.cpp'
> > > llvm-lit: <llvm>/llvm/utils/lit/lit/discovery.py:314: warning: input '<build>/test/libcxx/transitive_includes.gen.py/algorithm.sh.cpp' contained no tests
> > > error: did not discover any tests for provided path(s)
> > > ```
> > > 
> > > How can we run this test and update the transitive includes?
> > This would be fixed by D151664.
> Great! That patch was also very useful to test the new style tests for D151814.



================
Comment at: libcxx/test/libcxx/transitive_includes.gen.py:27-30
+# To re-generate the list of expected headers, temporarily set this to True, re-generate
+# the file and run this test.
+# Note that this needs to be done for all supported language versions of libc++:
+# for std in c++03 c++11 c++14 c++17 c++20 c++23 c++26; do <build>/bin/llvm-lit --param std=$std ${path_to_this_file}; done
----------------
ldionne wrote:
> Mordante wrote:
> > These instructions seem outdated an no longer working. The regeneration happen when I try to run this test. Unfortunately running the test fails.
> > 
> > `<build>/bin/llvm-lit --param std=c++03 test/libcxx/transitive_includes.gen.py` gives
> > ```
> > <llvm>/llvm/utils/lit/lit/discovery.py:189: error: 'llvm-libc++-shared.cfg.in :: libcxx/transitive_includes.gen.py' would not be run indirectly: change name or LIT config(e.g. suffixes or standalone_tests variables)
> > 1 errors, exiting
> > ```
> > 
> > Running the generated test manually 
> > `<build>/bin/llvm-lit <build>/build/test/libcxx/transitive_includes.gen.py/algorithm.sh.cpp`
> > gives
> > ```
> > 
> > llvm-lit: <llvm>/llvm/utils/lit/lit/discovery.py:137: warning: unable to find test suite for '<build>/test/libcxx/transitive_includes.gen.py/algorithm.sh.cpp'
> > llvm-lit: <llvm>/llvm/utils/lit/lit/discovery.py:314: warning: input '<build>/test/libcxx/transitive_includes.gen.py/algorithm.sh.cpp' contained no tests
> > error: did not discover any tests for provided path(s)
> > ```
> > 
> > How can we run this test and update the transitive includes?
> This would be fixed by D151664.
Great! That patch was also very useful to test the new style tests for D151814.


================
Comment at: libcxx/test/libcxx/transitive_includes.gen.py:36
+  print(f"""\
+//--- generate-transitive-includes.sh.cpp
+// RUN{BLOCKLIT}: mkdir %t
----------------
Mordante wrote:
> This file is generated when `regenerate_expected_results` is set to `True` but not removed when set to `False` again. I would like to test whether this generated test will be executed, but I can't seem to run this test. (See comment above.)
I tested this with D151664 and only the proper branch of `regenerate_expected_results` was executed. So this works as expected and the stale files are not an issue.


================
Comment at: libcxx/test/libcxx/transitive_includes.gen.py:88
+// RUN{BLOCKLIT}: %{{python}} %{{libcxx}}/test/libcxx/transitive_includes_to_csv.py %t/trace-includes.txt > %t/actual_transitive_includes.csv
+// RUN{BLOCKLIT}: cat %{{libcxx}}/test/libcxx/transitive_includes/%{{cxx_std}}.csv | awk '/^{escaped_header} / {{ print }}' > %t/expected_transitive_includes.csv
+// RUN{BLOCKLIT}: diff -w %t/expected_transitive_includes.csv %t/actual_transitive_includes.csv
----------------
ldionne wrote:
> Mordante wrote:
> > It seems we don't use awk in our tests yet. It's already a dependency since it's on of the POSIX tools. However I wonder why not using grep instead?
> That's a good question. I was using `grep` first.
> 
> `grep` returns non-zero when there are no matches, which fails this test for headers that have no transitive dependencies.
Ah interesting.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152008