Discover and run unit tests from check.py by tlively · Pull Request #1948 · WebAssembly/binaryen

tlively · 2019-03-16T03:43:11Z

unittest is Python's standard testing framework, so this change allows
arbitrary tests to be written without introducing any new dependencies
or code in check.py. A new test that was not possible to write before
is also included. It is the first of many.

unittest is Python's standard testing framework, so this change allows arbitrary tests to be written without introducing any new dependencies or code in check.py. A new test that was not possible to write before is also included. It is the first of many.

Refactors features into a new wasm-features.h file and updates the validator to check that all types are allowed. Currently this is only relevant for the v128 SIMD type, but new types will be added in the future. The test for this change is in WebAssembly#1948.

This is necessary to write tests that don't require temporary files, such as in WebAssembly#1948, and is generally useful.

tlively · 2019-03-16T04:02:31Z

+  result = Py2CompletedProcess(cmd, proc.returncode, stdout, stderr)
+  if check:
+    result.check_returncode()
+  return result


This is all copied from emscripten's shared.py. The only difference is that I added support for the capture_output parameter in run_process.

Should we add capture_output to emscripten's shared.py? (Not knowing what that enables in any detail)

It makes me sad that we need all this boilerplate..

It would be great to upgrade to python3 instead, but that should probably be a separate discussion.

@jgravelle-google I think we can wait to add capture_output to emscripten's shared.py until someone wants to use it. All it does is store the process's stdout and stderr on the returned object so they can be programmatically inspected. Emscripten tends to store output in temporary files instead.

tlively · 2019-03-16T04:02:35Z

    print '\n[ ' + str(num_failures) + ' failures! ]'

-  return num_failures
+  return 0 if num_failures == 0 else 1


This fixes a bug where if some multiple of 256 tests fail, the exit code would be 0 (on linux, anyway).

That should be a comment inline, because reversing this is exactly the sort of thing someone would think is a reasonable simplification

Because I don't like python's ternary syntax (maybe I'm biased), how about moving the return 1 into the block above as an early out and then just return 0 at the end here?

@jgravelle-google Good point. I'm not sure a comment makes sense anymore with the change @sbc100 suggested, though.

yay to code changes to make comments not-needed

Huh, yeah merging it with the num_failures clause above makes that even nicer 👍

tlively · 2019-03-16T04:03:28Z

+    )
+    """
+    p = run_process(WASM_OPT + ['--mvp-features', '--print'],
+                    input=module, check=False, capture_output=True)


This depends on #1950 to run.

This is necessary to write tests that don't require temporary files, such as in #1948, and is generally useful.

Refactors features into a new wasm-features.h file and updates the validator to check that all types are allowed. Currently this is only relevant for the v128 SIMD type, but new types will be added in the future. The test for this change is in #1948.

tlively · 2019-03-19T03:43:05Z

@kripken This has no more outstanding dependencies.

kripken

Overall lgtm but @sbc100 knows this stuff best.

kripken · 2019-03-19T16:38:35Z

+  print '\n[ checking unit tests...]\n'
+
+  # equivalent to `python -m unittest discover -s . -v`
+  suite = unittest.defaultTestLoader.discover(os.path.dirname(__file__))


would this look in all of binaryen? how about looking just in test/?

jgravelle-google

Generally lgtm, nice

jgravelle-google · 2019-03-19T17:13:04Z

+  result = Py2CompletedProcess(cmd, proc.returncode, stdout, stderr)
+  if check:
+    result.check_returncode()
+  return result


Should we add capture_output to emscripten's shared.py? (Not knowing what that enables in any detail)

sbc100 · 2019-03-19T18:48:58Z

 # limitations under the License.

 import os
+import unittest


sbc100 · 2019-03-19T18:51:36Z



+def run_unittest():
+  global num_failures


Hmm.. this seems odd. num_failures is a variable that is imported from scripts.test.shared but is a global in this file. Is this doing what you expect it to do? If we want changes to be visible in shared we would probably not use global by shared.num_failures.

Looking at the details of how num_failures is currently used, I think you probably want to use a new local here.. either that or somehow hook into the existing shared.fail_with_error system.

Yes, this is definitely doing what I expect it to do, which is count unittest failures as equivalent to any other failures. shared.num_failures is imported into the global namespace of this module, but shared is not, so using shared.num_failures instead of global num_failures would require a redundant import. Looking at shared.fail_with_error, it basically does the same thing but respects the abort_on_first_failure option. I will update this code to respect that option as well.

But the num_failures that is global the this module is different to one in shared isn't it?

Updating num_failures in shared won't effect this value and vice versa. I guess that bug already existed but this make it worse.

In python when you re-assign a global, it doesn't effect modules that have already imported that global.

Wow you're totally right. That's nuts. TIL. Fixing now.

... I take back some of the nice things I've said about Python

sbc100 · 2019-03-19T18:53:04Z

    print '\n[ ' + str(num_failures) + ' failures! ]'

-  return num_failures
+  return 0 if num_failures == 0 else 1


Because I don't like python's ternary syntax (maybe I'm biased), how about moving the return 1 into the block above as an early out and then just return 0 at the end here?

sbc100 · 2019-03-19T18:55:22Z

+  result = Py2CompletedProcess(cmd, proc.returncode, stdout, stderr)
+  if check:
+    result.check_returncode()
+  return result


It makes me sad that we need all this boilerplate..

tlively · 2019-03-20T00:24:09Z

Can I get an explicit lgtm on the new handling of options.abort_on_first_failure in run_unittest? Unfortunately I didn't see a good way to reuse the error handling in shared.py that properly counts the number of individual test failures.

tlively added a commit to tlively/binaryen that referenced this pull request Mar 16, 2019

Allow tools to read from stdin

4d21338

This is necessary to write tests that don't require temporary files, such as in WebAssembly#1948, and is generally useful.

This was referenced Mar 16, 2019

Validate that types match features #1949

Merged

Allow tools to read from stdin #1950

Merged

tlively commented Mar 16, 2019

View reviewed changes

tlively added a commit that referenced this pull request Mar 19, 2019

Allow tools to read from stdin (#1950)

a2dd7c4

This is necessary to write tests that don't require temporary files, such as in #1948, and is generally useful.

Merge branch 'master' of github.com:WebAssembly/binaryen into unittest

55196ae

Merge branch 'master' of github.com:WebAssembly/binaryen into unittest

263a468

tlively requested a review from kripken March 19, 2019 03:43

kripken reviewed Mar 19, 2019

View reviewed changes

kripken requested a review from sbc100 March 19, 2019 16:46

jgravelle-google approved these changes Mar 19, 2019

View reviewed changes

sbc100 approved these changes Mar 19, 2019

View reviewed changes

Address comments

6615abe

Fix importing of shared.num_failures

bf22b66

sbc100 approved these changes Mar 20, 2019

View reviewed changes

tlively merged commit fe0b16a into WebAssembly:master Mar 20, 2019

tlively deleted the unittest branch April 24, 2020 23:26

Conversation

tlively commented Mar 16, 2019

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!

tlively commented Mar 19, 2019

Uh oh!

kripken left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgravelle-google 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!

tlively commented Mar 20, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants