Introduce lit/FileCheck tests by tlively · Pull Request #3367 · WebAssembly/binaryen

tlively · 2020-11-15T05:34:32Z

lit and FileCheck are the tools used to run the majority of tests in LLVM. Each
lit test file contains the commands to be run for that test, so lit tests are
much more flexible and can be more precise than our current ad hoc testing
system. FileCheck reads expected test output from comments, so it allows test
output to be written alongside and interspersed with test input, making tests
more readable and precise than in our current system.

This PR adds a new suite to check.py that runs lit tests in the test/lit
directory. A few tests have been ported to demonstrate the features of the new
test runner.

This change is motivated by a need for greater flexibility in testing wasm-split.
See #3359.

tlively · 2020-11-15T05:38:36Z

@kripken @aheejin @dschuff @MaxGraey What would you think about this direction? I would rather use lit + FileCheck for wasm-split (#3359) and invest time in porting the other tests to lit + FileCheck than invest time in writing a new ad hoc test script for wasm-split.

tlively · 2020-11-15T21:37:08Z

@sbc100 why a new check.py option was necessary in #3359, but I will answer here to keep the respective discussions more focused.

Most users will never have to use this option, even if they do out-of-tree builds, since it is only necessary when --binaryen-bin is set to the install directory rather than the build directory. The way it is currently written, CMake emits a lit configuration file that tells lit where to find the built binaries and where to find the tests. It would be possible to have this configuration file point to the binaries in the install directory rather than the build directory, but check.py would still have to know how to find the lit configuration file itself. Between the options of installing the lit configuration to the install directory and adding a new check.py option to cover this corner case, adding the option seemed better.

kripken · 2020-11-15T22:26:55Z

Personally this sounds fine to me. I'm not worried about having multiple test frameworks, as long as they are all very simple and they all are useful in their own right. (I'd feel very differently about non-test code!)

My main concern about lit is how easy will it be to update test expectations - can we get ./auto_update_tests to do everything automatically like it does now even for those tests? Sounds from the above like that's the plan. But it would require "clever" diffing it seems?

My small concern about lit is that while it's nice to see the input and output in the same file, the output is in a comment. That's less readable and also it's not directly runnable. For example, right now you can run an input wast through a tool, then run the output wast through the same, and compare results. With lit you'd need to construct the output wast somehow from those comments..? If we don't have a great solution for that, I'd still be fine with using lit for some tests where running the output is not that important - but it would make me skeptical of the benefit of porting existing tests to lit (but leaving them as is sounds fine to me anyhow as I said earlier).

tlively · 2020-11-15T23:32:40Z

My main concern about lit is how easy will it be to update test expectations - can we get ./auto_update_tests to do everything automatically like it does now even for those tests? Sounds from the above like that's the plan. But it would require "clever" diffing it seems?

Yes, the auto update scripts would be more complex for lit tests. But LLVM has such scripts, so at the very least we could use those as a starting point.

My small concern about lit is that while it's nice to see the input and output in the same file, the output is in a comment. That's less readable and also it's not directly runnable. For example, right now you can run an input wast through a tool, then run the output wast through the same, and compare results. With lit you'd need to construct the output wast somehow from those comments..? If we don't have a great solution for that, I'd still be fine with using lit for some tests where running the output is not that important - but it would make me skeptical of the benefit of porting existing tests to lit (but leaving them as is sounds fine to me anyhow as I said earlier).

It should be trivial to construct the output by running the commands in the ;; RUN: lines of the tests. Also, when a test fails, lit will print out the exact commands to run to reproduce the failure yourself, so I expect that reproducing output will be at least as easy with lit tests as with current tests.

I still think that porting existing tests to lit will be a huge win once we have auto update scripts for lit tests, but if you're ok with adding lit as a new test runner, we can use it for wasm-split for now and defer the decision on whether to automatically port other tests until we have a more concrete idea of how good the results would be.

sbc100 · 2020-11-16T03:03:21Z

@sbc100 why a new check.py option was necessary in #3359, but I will answer here to keep the respective discussions more focused.

Most users will never have to use this option, even if they do out-of-tree builds, since it is only necessary when --binaryen-bin is set to the install directory rather than the build directory. The way it is currently written, CMake emits a lit configuration file that tells lit where to find the built binaries and where to find the tests. It would be possible to have this configuration file point to the binaries in the install directory rather than the build directory, but check.py would still have to know how to find the lit configuration file itself. Between the options of installing the lit configuration to the install directory and adding a new check.py option to cover this corner case, adding the option seemed better.

Can we simplify things my mandating that tests can only run against a build directory and never against an install directory?

sbc100 · 2020-11-16T03:18:04Z

My main concern about lit is how easy will it be to update test expectations - can we get ./auto_update_tests to do everything automatically like it does now even for those tests? Sounds from the above like that's the plan. But it would require "clever" diffing it seems?

Yes, the auto update scripts would be more complex for lit tests. But LLVM has such scripts, so at the very least we could use those as a starting point.

I think we if we make this transition we can/should do it incrementally.

If there are certainly test suites for which it makes sense to stick with the "expected output is this entire" file approach I think that is perfectly reasonable. I think its fine for there to exist two catagories of tests:

Check just specific lines, and do so via inline comments.
Check entire output and do so using a separate file.

Trying to make all tests fit into (1) doesn't make sense to me (at least not yet). But I can't see that for many tests it is clearly a better option.

I am not particularly excited about having arbitrarily complex scripts that try to make the auto-update system work with (1). I might make sense for certain tests and not others.

My small concern about lit is that while it's nice to see the input and output in the same file, the output is in a comment. That's less readable and also it's not directly runnable. For example, right now you can run an input wast through a tool, then run the output wast through the same, and compare results. With lit you'd need to construct the output wast somehow from those comments..? If we don't have a great solution for that, I'd still be fine with using lit for some tests where running the output is not that important - but it would make me skeptical of the benefit of porting existing tests to lit (but leaving them as is sounds fine to me anyhow as I said earlier).

It should be trivial to construct the output by running the commands in the ;; RUN: lines of the tests. Also, when a test fails, lit will print out the exact commands to run to reproduce the failure yourself, so I expect that reproducing output will be at least as easy with lit tests as with current tests.

I still think that porting existing tests to lit will be a huge win once we have auto update scripts for lit tests, but if you're ok with adding lit as a new test runner, we can use it for wasm-split for now and defer the decision on whether to automatically port other tests until we have a more concrete idea of how good the results would be.

I think we should do this incrementally, that way we can be exposed different pain points of the two systems side by side side.

My feeling is that biggest issue with the current system is the lack of ability to run individual tests, or even know how to re-run a given test when something fails.. or even tell from the output which test exactly failed.. mostly because tests do not have unique names or IDs. While lit + FileCheck conversion could solve that issue, it could also be solved in other ways. For example, Wouter's --filter option helps in this direction.

tlively · 2020-11-16T23:10:46Z

@aheejin, a brief investigation in #3375 shows that the clang package we are already bringing in for the lint builder does not include llvm-lit or FileCheck. We also don't install that package on Mac or Windows, so I think for now the best thing to do is to continue using the python packages since we already know they can be installed and are portable.

sbc100 · 2020-11-16T23:17:41Z

@aheejin, a brief investigation in #3375 shows that the clang package we are already bringing in for the lint builder does not include llvm-lit or FileCheck. We also don't install that package on Mac or Windows, so I think for now the best thing to do is to continue using the python packages since we already know they can be installed and are portable.

BTW we should probably use requirements-dev.txt like we do in emscripten to capture these dependencies.

tlively · 2020-11-17T00:02:38Z

@sbc100 sounds good, but let's put that in a separate PR that also adds flake8 to that file.

sbc100 · 2020-11-17T00:04:53Z

Unless you are in a hurry I suggest we move flake8 to that file first.. thus avoiding the need to change the CI script at all for this change.

tlively · 2020-11-17T00:17:06Z

Oh, does github actions know about that file or something?

sbc100 · 2020-11-17T00:19:43Z

No we just do pip3 install -r requirements-dev.txt

This file makes it simple for users and CI bots to install all the Python dev dependencies necessary to run the test suite. Right now it only contains flake8, but soon it will contain lit and filecheck as well (see WebAssembly#3367).

This file makes it simple for users and CI bots to install all the Python dev dependencies necessary to run the test suite. Right now it only contains flake8, but soon it will contain lit and filecheck as well (see #3367).

lit and FileCheck are the tools used to run the majority of tests in LLVM. Each lit test file contains the commands to be run for that test, so lit tests are much more flexible and can be more precise than our current ad hoc testing system. FileCheck reads expected test output from comments, so it allows test output to be written alongside and interspersed with test input, making tests much more readable than in our current system. This PR adds a new suite to check.py that runs lit tests in the test/lit directory. The only such test so far contains a few test cases ported from the optimize-instructions_all-features test, meant to demonstrate what our tests would look like if they were ported to lit and FileCheck. To avoid adding yet another testing framework (https://xkcd.com/927/), I would like to try to port our existing tests to use lit and FileCheck. This should be done at least partially automatically by scripts that will remain useful in the future for updating test expectations.

sbc100

This is looking very nice indeed!

sbc100 · 2020-11-17T16:16:05Z

 *.o
 *.obj
 compile_commands.json
+**/Output


Just /Output should be enough, no?

No, lit actually sprinkles Output directories all over the place. After running the tests in this PR locally with an in-tree build I have

./test/Output ./test/validation/Output ./test/wasm-emscripten-finalize/Output

Cluttering test directory does not sound very ideal.. Can we put all of them under out/ or somewhere? There seems to be an option called test_exec_root in llvm-lit with which we can control the output directory. Can we use it?

These files get written to the build directory, so it only clutters your source tree if your do "in-tree" building.. which I think most of us dont.

If you do do "in-tree" building your source tree is already full of clutter.

If we banned in-tree building we could remove all these things from gitignore.. which I would personally be an favor of. I personally like it when git status tells me about stray .o files in my checkout, but these rules prevent that.

sbc100 · 2020-11-17T16:16:21Z

 *.obj
 compile_commands.json
+**/Output
+**/lit.site.cfg.py


Are these files created during the build? Where are they created exactly?

This file is generated at build configuration time by CMake. For a normal in-tree build, this is created at ./test/lit/lit.site.cfg.py, but it's relative to the CMake build directory rather than the source directory.

Got it. In that case it should just be /test/lit/lit.site.cfg.py since it has a fixed location.

sbc100 · 2020-11-17T16:17:21Z

  E241, # space after comma (ignored for list in gen-s-parser.py)
  W504  # line break after binary operator
-exclude = ./test/emscripten,./test/spec,./test/wasm-install
+exclude = ./test/emscripten,./test/spec,./test/wasm-install,./test/lit


flake8 complains about config being used before it is defined in the lit config and similar non-problems.

How about test/lib/*config.py or something like that then? Or will this directory be otherwise free of python code so it doesn't matter?

Yes, this directory will otherwise be free of Python code.

sbc100 · 2020-11-17T16:19:27Z

@@ -0,0 +1,20 @@
+# Copyright 2020 WebAssembly Community Group participants


#! python3?

See mull-project/FileCheck.py#149 for more info.

aheejin

It looks very nice! As I said, I'm not sure whether migrating all existing tests is feasible or better; there are cases we change some parts of existing passes that affect many existing tests and running ./auto_update_tests.py can often show what its effects would be like on many parts on them, which I found rather convenient. But having this as an alternative testing framework looks fine.

aheejin · 2020-11-18T09:11:18Z

 *.o
 *.obj
 compile_commands.json
+**/Output


Cluttering test directory does not sound very ideal.. Can we put all of them under out/ or somewhere? There seems to be an option called test_exec_root in llvm-lit with which we can control the output directory. Can we use it?

aheejin · 2020-11-18T09:50:48Z


 flake8==3.7.8
+filecheck==0.0.17
+lit==0.11.0.post1


I'm not familiar with Python ports of filecheck and lit... So lit needs a wrapper script but filecheck can run as is like a normal executable?

kripken

No update to ./auto_update_test.py for lit tests? 😜

tlively · 2020-11-18T17:43:55Z

No update to ./auto_update_test.py for lit tests? 😜

Not for these hand-written ones :P

tlively · 2020-11-18T19:10:56Z

@aheejin, nice find with the test_exec_root. You were right that I could move the Output directories into out/test/ with that.

tlively mentioned this pull request Nov 15, 2020

Initial wasm-split tool #3359

Merged

sbc100 closed this Nov 16, 2020

sbc100 reopened this Nov 16, 2020

tlively mentioned this pull request Nov 17, 2020

Add requirements-dev.txt #3377

Merged

tlively added 7 commits November 16, 2020 18:35

Require binaryen-bin to be in the build dir

38d07e7

Add substitution for all tools

2a41c84

Restore original test and add not.py

3f4e3e5

Use requirements-dev.txt

f770dc4

Port bigint wasm-emscripten-finalize test

9b09a29

remove optimize-instructions-arithmetic.wast

1312ec2

tlively force-pushed the lit-filecheck branch from 816312e to 1312ec2 Compare November 17, 2020 02:36

tlively added 2 commits November 16, 2020 19:00

Port some unit tests that check errors

38d2b0d

Fix whitespace errors

e5cdc48

tlively changed the title ~~[Discussion] lit/FileCheck tests~~ Introduce lit/FileCheck tests Nov 17, 2020

Fix flake8 errors

1415f22

tlively added 7 commits November 16, 2020 19:06

Spelling

6f943e3

Debug Windows failure

fbe4a5c

fix

a7fd790

Windows fix

3b9e60f

sys.executable to run not.py

4103db0

Forward slashes again

dfe3b9e

reorganize gitignore additions

0a4381d

tlively marked this pull request as ready for review November 17, 2020 07:27

sbc100 reviewed Nov 17, 2020

View reviewed changes

Add shebang to lit_wrapper.py

66be4b2

tlively requested review from kripken and sbc100 November 17, 2020 20:28

tlively added 2 commits November 17, 2020 12:37

More specific .gitignore

e48cba4

Update filecheck to allow similar prefixes

4700d02

See mull-project/FileCheck.py#149 for more info.

aheejin approved these changes Nov 18, 2020

View reviewed changes

kripken approved these changes Nov 18, 2020

View reviewed changes

Move Ouput to out/test

b51d48f

tlively merged commit 1e527ec into WebAssembly:master Nov 18, 2020

tlively deleted the lit-filecheck branch November 18, 2020 19:27

		@@ -0,0 +1,20 @@
		# Copyright 2020 WebAssembly Community Group participants

Conversation

tlively commented Nov 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tlively commented Nov 15, 2020

Uh oh!

tlively commented Nov 15, 2020

Uh oh!

kripken commented Nov 15, 2020

Uh oh!

tlively commented Nov 15, 2020

Uh oh!

sbc100 commented Nov 16, 2020

Uh oh!

sbc100 commented Nov 16, 2020

Uh oh!

tlively commented Nov 16, 2020

Uh oh!

sbc100 commented Nov 16, 2020

Uh oh!

tlively commented Nov 17, 2020

Uh oh!

sbc100 commented Nov 17, 2020

Uh oh!

tlively commented Nov 17, 2020

Uh oh!

sbc100 commented Nov 17, 2020

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aheejin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kripken left a comment

Choose a reason for hiding this comment

Uh oh!

tlively commented Nov 18, 2020

Uh oh!

tlively commented Nov 18, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tlively commented Nov 15, 2020 •

edited

Loading