Skip tests that fail on windows and enable all the rest by kripken · Pull Request #3035 · WebAssembly/binaryen

kripken · 2020-08-10T18:14:32Z

This lets us run most tests at least on that platform.

Add a new function for skipping those tests, skip_if_on_windows,
so that it's easy to find which tests are disabled on windows for later fixing
efforts.

This fixes a few minor issues for windows, like comparisons
should ignore \r in some cases.

aheejin

Why are _dwarf added to test file names..? Do we have a pass called --dwarf?

aheejin · 2020-08-10T19:59:01Z

+    if get_platform() == 'windows':
+        print('skipping test "%s" on windows' % name)
+        return True
+    return False


Doesn't this mean we don't run anything on Windows? Is this only a placeholder for now and are we planning to add real conditions here later? If so, where is this supposed to be used? Currently it seems it is used only in spec tests and wasm-opt pass tests... Is there a guideline where we should use this or not?

The idea is that this would be called when we know a test currently fails on windows. It just returns true/false so it's up to the calling code to actually skip the test.

I plan to use it in this PR to disable all failing windows tests, which should let us enable the majority of tests at least (I hope).

What I meant was, this returns True for everything on Windows now even without checking names.. So do you mean this is a placeholder for now and you are gonna add real file names later?

No, this is the actual function. It's only called when we should definitely skip what we are currently doing if we are on windows. So the caller decides which test or part of a test to skip, and this just logs and returns True, and provides a simple way to search for all windows skips.

See e.g. the wasm2js test change https://github.com/WebAssembly/binaryen/pull/3035/files#diff-611fb24eb3395253b8c42c20f7238e8e

Oh, I see. Thanks. I was confused.

aheejin · 2020-08-10T20:01:17Z

+        base = os.path.basename(wast)
+        print('..', base)
+        # windows has some failures that need to be investigated
+        if base == 'names.wast' and shared.skip_if_on_windows('spec: ' + base):


Why skip names.wast? If this is not something windows-related, we have something similar in shared.py:

binaryen/scripts/test/shared.py

Lines 385 to 440 in 454a1cb

SPEC_TEST_BLACKLIST = [

# Stacky code / notation

'block.wast',

'call.wast',

'float_exprs.wast',

'globals.wast',

'loop.wast',

'nop.wast',

'select.wast',

'stack.wast',

'unwind.wast',

# Binary module

'binary.wast',

'binary-leb128.wast',

'custom.wast',

# Empty 'then' or 'else' in 'if'

'if.wast',

'local_set.wast',

'store.wast',

# No module in a file

'token.wast',

'utf8-custom-section-id.wast',

'utf8-import-field.wast',

'utf8-import-module.wast',

'utf8-invalid-encoding.wast',

# 'register' command

'imports.wast',

'linking.wast',

# Misc. unsupported constructs

'call_indirect.wast', # Empty (param) and (result)

'const.wast', # Unparenthesized expression

'data.wast', # Various unsupported (data) notations

'elem.wast', # Unsupported 'offset' syntax in (elem)

'exports.wast', # Multiple inlined exports for a function

'func.wast', # Forward named type reference

'skip-stack-guard-page.wast', # Hexadecimal style (0x..) in memory offset

# Untriaged: We don't know the cause of the error yet

'address.wast', # wasm2js 'assert_return' failure

'br_if.wast', # Validation error

'float_literals.wast', # 'assert_return' failure

'int_literals.wast', # 'assert_return' failure

'local_tee.wast', # Validation failure

'memory_grow.wast', # 'assert_return' failure

'start.wast', # Assertion failure

'type.wast', # 'assertion_invalid' failure

'unreachable.wast', # Validation failure

'unreached-invalid.wast' # 'assert_invalid' failure

]

options.spec_tests = [t for t in options.spec_tests if os.path.basename(t) not

in SPEC_TEST_BLACKLIST]

Hmm, maybe it should be added to that list then, good point. It's windows-specific but we could append it when on that platform. However, then we can't use the skip_if_on_windows function which otherwise would be a single easy way to find all skipped stuff in windows... what's better?

Oh, sorry, I didn't see names.wast is skipped only on Windows.. I thought it was an 'or' condition, so I thought the condition was if it is names.wast or it is on Windows. This part of the code excludes failing tests from the list of spec tests, so if we add some platform handling logic I think we can make those files be excluded from here. But if we have only a single case of names.wast that is platform-dependent, I'm not sure that's worth it. Also this list is only for spec tests, so maybe it'd be better to keep them separate after all, unless we make a more complex system of skipping tests that takes the platform and the kind of tests as arguments, which seems like an overkill.

Seems like overkill to me too, to try to generalize this. I don't feel strongly, but I think I prefer the current status of the code.

kripken · 2020-08-10T20:30:17Z

@aheejin There is a pass called "dwarfdump" which prints out the DWARF, and is what we need to skip on windows for now.

aheejin · 2020-08-10T21:26:19Z

@aheejin There is a pass called "dwarfdump" which prints out the DWARF, and is what we need to skip on windows for now.

Sorry I still don't understand how it is related to the file renaming..

kripken · 2020-08-10T21:55:53Z

Oh, the name of the test may not contain the passes we run. So I renamed tests that contain a call to --dwarfdump to contain dwarf in the name. That seems clearer anyhow I think, aside from making it easy to skip the dwarf tests here.

kripken · 2020-08-10T22:28:43Z

Ok, I think the work here is done - I had to disable a bunch of things like wasm2js and unit tests, but the majority do pass now.

aheejin · 2020-08-11T05:28:55Z

 # delete the old file, make sure you rename the corresponding .wast.log file in
 # expected-output/ if any.
-SPEC_TEST_BLACKLIST = [
+SPEC_TESTS_TO_SKIP = [


Thanks! I forgot we still had this.

aheejin · 2020-08-11T05:30:34Z

+    if get_platform() == 'windows':
+        print('skipping test "%s" on windows' % name)
+        return True
+    return False


Oh, I see. Thanks. I was confused.

aheejin · 2020-08-11T05:48:15Z

+
+    # similar to assertEqual, but while ignoring line ending differences such
+    # as those between windows and unix
+    def assert_equal_ignoring_line_endings(self, left, right):


We don't need to replace other usages of assert_equal with this?

I think we would need to audit each one to see if line endings should be ignored there or not. That may explain some of the remaining failures in unit, but not the first few at least (which is when I gave up on trying to fix them all).

kripken added 4 commits August 10, 2020 10:50

disable some tests

3284035

fixes

4c093d1

fix

f9c937c

enable testing

7381db8

kripken changed the title ~~Skip tests that fail on windows~~ Skip tests that fail on windows and enable all the rest Aug 10, 2020

kripken added 4 commits August 10, 2020 11:40

renames

bc50575

fix

b9953a4

rename

6351fa6

another

bf2efdc

aheejin reviewed Aug 10, 2020

View reviewed changes

kripken added 2 commits August 10, 2020 13:02

wasm2js

98df8b1

gcc

d1ff96b

fix a unit test for windows and other cleanups

7a0be5b

fix a unit test maybe

3ea2218

kripken added 2 commits August 10, 2020 15:10

unit

3394725

fix

00917eb

aheejin approved these changes Aug 11, 2020

View reviewed changes

kripken merged commit f067a45 into master Aug 11, 2020

kripken deleted the wintest branch August 11, 2020 17:31

	SPEC_TEST_BLACKLIST = [
	# Stacky code / notation
	'block.wast',
	'call.wast',
	'float_exprs.wast',
	'globals.wast',
	'loop.wast',
	'nop.wast',
	'select.wast',
	'stack.wast',
	'unwind.wast',

	# Binary module
	'binary.wast',
	'binary-leb128.wast',
	'custom.wast',

	# Empty 'then' or 'else' in 'if'
	'if.wast',
	'local_set.wast',
	'store.wast',

	# No module in a file
	'token.wast',
	'utf8-custom-section-id.wast',
	'utf8-import-field.wast',
	'utf8-import-module.wast',
	'utf8-invalid-encoding.wast',

	# 'register' command
	'imports.wast',
	'linking.wast',

	# Misc. unsupported constructs
	'call_indirect.wast', # Empty (param) and (result)
	'const.wast', # Unparenthesized expression
	'data.wast', # Various unsupported (data) notations
	'elem.wast', # Unsupported 'offset' syntax in (elem)
	'exports.wast', # Multiple inlined exports for a function
	'func.wast', # Forward named type reference
	'skip-stack-guard-page.wast', # Hexadecimal style (0x..) in memory offset

	# Untriaged: We don't know the cause of the error yet
	'address.wast', # wasm2js 'assert_return' failure
	'br_if.wast', # Validation error
	'float_literals.wast', # 'assert_return' failure
	'int_literals.wast', # 'assert_return' failure
	'local_tee.wast', # Validation failure
	'memory_grow.wast', # 'assert_return' failure
	'start.wast', # Assertion failure
	'type.wast', # 'assertion_invalid' failure
	'unreachable.wast', # Validation failure
	'unreached-invalid.wast' # 'assert_invalid' failure
	]
	options.spec_tests = [t for t in options.spec_tests if os.path.basename(t) not
	in SPEC_TEST_BLACKLIST]

Conversation

kripken commented Aug 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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 Aug 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kripken commented Aug 10, 2020

Uh oh!

aheejin commented Aug 10, 2020

Uh oh!

kripken commented Aug 10, 2020

Uh oh!

kripken commented Aug 10, 2020

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kripken commented Aug 10, 2020 •

edited

Loading

aheejin Aug 10, 2020 •

edited

Loading