Implement a simple test filter #35

yamt · 2022-12-19T05:55:48Z

loganek · 2023-01-03T16:26:40Z

README.md

@@ -38,6 +38,16 @@ python3 test-runner/wasi_test_runner.py
    -r adapters/wasmtime.sh # path to a runtime adapter
 ```

+Optianally you can specify test cases to skip with the `--filter` option.


Suggested change

Optianally you can specify test cases to skip with the `--filter` option.

Optionally you can specify test cases to skip with the `--filter` option.

loganek · 2023-01-03T16:28:46Z

README.md

+python3 test-runner/wasi_test_runner.py                                                  \
+    -t ./tests/assemblyscript/testsuite/ `# path to folders containing .wasm test files` \
+       ./tests/c/testsuite/                                                              \
+    --filter examples/skip.json


Suggested change

--filter examples/skip.json

--filter examples/skip.json \

loganek · 2023-01-03T16:31:06Z

test-runner/wasi_test_runner/reporters/console.py

@@ -54,6 +54,7 @@ def finalize(self, version: RuntimeVersion) -> None:
            print(f"  Total: {suite.test_count}")
            self._print_pass(f"  Passed:  {suite.pass_count}")
            self._print_fail(f"  Failed:  {suite.fail_count}")
+            print(f"  Skipped: {suite.skip_count}")


Suggested change

print(f" Skipped: {suite.skip_count}")

self._print_skip(f" Skipped: {suite.skip_count}")

loganek · 2023-01-03T16:34:56Z

test-runner/tests/test_test_suite_runner.py

    with patch("glob.glob", return_value=test_paths):
-        suite = tsr.run_tests_from_test_suite("my-path", runtime, validators, reporters)  # type: ignore
+        suite = tsr.run_tests_from_test_suite("my-path", runtime, validators, reporters, filters)  # type: ignore


should we validate if filters actually were called?

it'd be also good to validate the case when the filter returns False and in that case the test is skipped

loganek · 2023-01-03T16:38:21Z

examples/skip.json

@@ -0,0 +1,5 @@
+{


I'm not sure if we actually need such a detailed filters for now. Initially I was thinking of the following requirements:

being able to do both: include and exclude tests (e.g. --filter-include ".*thread.*" --filter-exclude ".*open.*")

pass regular expressions so we can e.g. disable the whole testsuite (something like --filter-exclude "WASI C tests.*")

But I'm curious what are your thoughts on that.

my intention is to implement something simplest first. (regex etc is more complex and advanced in my taste.)

also i think it makes sense to have a filter in a file because i guess it's almost static for a given runtime.

Sure, I don't suggest implementing all that at once, but I'd like us to think a bit about the API before we move forward. The proposed API of the JSON file only allows for disabling tests, and I'm afraid that if we want to add more features (like the ones mentioned above) we'll either have to make backward-incompatible changes or make the API inconsistent. As I said, we don't have to implement everything in the first iteration, but at least having placeholders would probably simplify further development.

i feel compatibility concern is not too important at this point.

also, your requirements are not clear to me.
if you explain it a bit more, maybe i can suggest a schema.

Sure; so first of all, I think we should be able to allow users to both select exclusion and inclusion filter (at the moment only the exclusion is possible). Also, we might allow for wildcard/regex, but I assume this could be just a feature request for the future, and we can implement it in the future; just an idea (although feel free to come up with something different):

{ "include": [], // if empty, all tests are included "exclude/ignore(whatever sounds better)": [] // if empty, none of the tests are excluded }

Element in the array could be a full path to a test, e.g. WASI C tests.clock_getres-monotonic, so we can in the future implement wildcards to only enable clock_getres tests, with: WASI C tests.clock_getres*.

As I said, this is just an example schema to explain what I mean; please let me know if it's clear what we try to achieve here and share your ideas.

my suggestion for this PR:

rename --filter option to --exclude-filter

no changes to schema

we can consider adding --include-filter, --exclude-regex-filter etc later when/if necessary.

rename --filter option to --exclude-filter

Given we already define filters in the JSON file, I don't know if there's a need for having two cli parameters, or instead could we have just one (--filter) and update the schema so it allows for specifying both inclusion and exclusion - I don't have a strong opinion; just a thought, as I haven't seen them separate in other farameworks.

no changes to schema

Not sure if we actually need a nested structure here (testsuite->tests); if we implement regex/glob we could probably just have a list of strings? We can start with this one as it might seem a bit simpler but once we update it, we'll have to update all the consumers as well (not sure how many runtimes will onboard by then though).

I'm also not 100% sure about file vs cli parameter. Whereas I see the benefit you mentioned above, I think it
doesn't matter that much for runtimes as they'll have their own CI scripts that wrap around the call to the tests. The benefit of having everything through CLI is that developers can easily just filter some of the tests they're currently working on without modifying file (and risking commiting it accidentally). Similar to my previous point, it is something we can change in the future, but depending on the number of adopters, it might be easier or harder.

rename --filter option to --exclude-filter

Given we already define filters in the JSON file, I don't know if there's a need for having two cli parameters, or instead could we have just one (--filter) and update the schema so it allows for specifying both inclusion and exclusion - I don't have a strong opinion; just a thought, as I haven't seen them separate in other farameworks.

given the current cli options which allows to specify multiple testsuites,
i thought it's natural to accept multiple filters.

no changes to schema

Not sure if we actually need a nested structure here (testsuite->tests); if we implement regex/glob we could probably just have a list of strings? We can start with this one as it might seem a bit simpler but once we update it, we'll have to update all the consumers as well (not sure how many runtimes will onboard by then though).

i'm not the person who invented the testsuite->tests structure in this repo.
honestly speaking i don't think the testsuite concept makes much sense.

maybe we can use some kind of string match with test id, where test id is:

test_id = testsuite + '/' + test

I'm also not 100% sure about file vs cli parameter. Whereas I see the benefit you mentioned above, I think it doesn't matter that much for runtimes as they'll have their own CI scripts that wrap around the call to the tests. The benefit of having everything through CLI is that developers can easily just filter some of the tests they're currently working on without modifying file (and risking commiting it accidentally). Similar to my previous point, it is something we can change in the future, but depending on the number of adopters, it might be easier or harder.

as a developer, i feel it easier to copy/modify a file than tweaking cli options.
but i can understand you have a different preference.

To unblock the change, let's move on with your suggestion here #35 (comment), we'll adapt it once we have more feedback.

To unblock the change, let's move on with your suggestion here #35 (comment), we'll adapt it once we have more feedback.

done

cf. WebAssembly#29

loganek · 2023-01-11T10:27:22Z

test-runner/wasi_test_runner/filters.py

+        with open(filename, encoding="utf-8") as file:
+            self.filter_dict = json.load(file)
+
+    def should_skip(self, test_suite_name: str, test_name: str) -> Tuple[bool, Any]:


Should that be str instead of Any?

i made it Any because it can be None.

In that case it should be Tuple[bool, Optional[str]]

iirc such complex types are rejected by tools i was using when writing this code. i guess it's better to avoid being too eager for now.

What are those tools? I think mypy should be able to handle that. Unless we really except passing here something other than str or None, I'd rather stick to precise types. That also helps understanding the code; in fact, I wasn't sure what is being returned here until I've looked to the implementation.

What are those tools? I think mypy should be able to handle that. Unless we really except passing here something other than str or None, I'd rather stick to precise types. That also helps understanding the code; in fact, I wasn't sure what is being returned here until I've looked to the implementation.

i think it was mypy. but i don't remember.

We already have a few static code analyzers as part of CI (including mypy); I'd suggest giving it a try, and if for some reason it fails, we can open an issue for mypy and switch back to Any for now.

i tried more strict annotation and for some reasons it worked today.

maybe the annotation which didn't work was different from this version. i don't remember.

i tried more strict annotation and for some reasons it worked today.

maybe the annotation which didn't work was different from this version. i don't remember.

now i remember. the mypy version used by the ci doesn't handle these complex type annotations well. i've updated the requirement.

loganek · 2023-01-11T10:29:34Z

test-runner/wasi_test_runner/test_suite_runner.py

+    return TestCase(
+        name=os.path.splitext(os.path.basename(test_path))[0],
+        config=config,
+        result=Result(output=Output(0, "", ""), is_executed=False, failures=[]),


Should the output be optional? Or rather have an union of output|skip_reason and the value is picked up based on is_executed

maybe. i'd like to postpone it for later PRs.

it would be nicer to be able to represent "timed out" as well. #42

Alright, we can refactor that piece as part of #42 then.

…rict

loganek

LGTM with a minor comment regarding tests

As it seems necessary for "test-runner/wasi_test_runner/filters.py: make type annotation more strict" ``` wasi_test_runner/filters.py:26: error: Incompatible return value type (got "Tuple[bool, None]", expected "Union[Tuple[Literal[True], str], Tuple[Literal[False], None]]") [return-value] wasi_test_runner/filters.py:29: error: Incompatible return value type (got "Tuple[bool, Any]", expected "Union[Tuple[Literal[True], str], Tuple[Literal[False], None]]") [return-value] wasi_test_runner/filters.py:30: error: Incompatible return value type (got "Tuple[bool, None]", expected "Union[Tuple[Literal[True], str], Tuple[Literal[False], None]]") [return-value] ```

yamt · 2023-01-20T02:03:15Z

@loganek are you waiting for fixes for your "minor comment"?

loganek · 2023-01-20T11:54:38Z

I'm ok with merging that as it is but I'd like to see a bit more tests. Let me know if you can add them, otherwise I'll open an issue and might pick that up later.

yamt force-pushed the skip branch 5 times, most recently from 41758ae to 70075c3 Compare December 19, 2022 08:19

loganek requested changes Jan 3, 2023

View reviewed changes

yamt force-pushed the skip branch 2 times, most recently from 7afac23 to 10b4115 Compare January 4, 2023 05:05

yamt added 2 commits January 11, 2023 11:58

Implement a simple test filter

e400d03

cf. WebAssembly#29

Rename --filter to --exclude-filter

5b2eee3

yamt force-pushed the skip branch from 10b4115 to 5b2eee3 Compare January 11, 2023 03:06

loganek reviewed Jan 11, 2023

View reviewed changes

test-runner/wasi_test_runner/filters.py: make type annotation more st…

9627233

…rict

loganek approved these changes Jan 11, 2023

View reviewed changes

loganek merged commit babca49 into WebAssembly:main Jan 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement a simple test filter #35

Implement a simple test filter #35

yamt commented Dec 19, 2022

loganek Jan 3, 2023

yamt Jan 4, 2023

loganek Jan 3, 2023

yamt Jan 4, 2023

loganek Jan 3, 2023

yamt Jan 4, 2023

loganek Jan 3, 2023

yamt Jan 4, 2023

loganek Jan 11, 2023

loganek Jan 3, 2023

yamt Jan 4, 2023

loganek Jan 4, 2023

yamt Jan 4, 2023

loganek Jan 5, 2023

yamt Jan 6, 2023

loganek Jan 6, 2023 •

edited

Loading

yamt Jan 6, 2023

loganek Jan 6, 2023

yamt Jan 11, 2023

loganek Jan 11, 2023

yamt Jan 11, 2023

loganek Jan 11, 2023

yamt Jan 11, 2023

loganek Jan 11, 2023

yamt Jan 11, 2023

loganek Jan 11, 2023

yamt Jan 11, 2023

yamt Jan 11, 2023

loganek Jan 11, 2023

yamt Jan 11, 2023

loganek Jan 11, 2023

loganek left a comment

yamt commented Jan 20, 2023

loganek commented Jan 20, 2023

	Optianally you can specify test cases to skip with the `--filter` option.
	Optionally you can specify test cases to skip with the `--filter` option.

	print(f" Skipped: {suite.skip_count}")
	self._print_skip(f" Skipped: {suite.skip_count}")

Implement a simple test filter #35

Implement a simple test filter #35

Conversation

yamt commented Dec 19, 2022

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

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

loganek Jan 6, 2023 • edited Loading

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

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

loganek left a comment

Choose a reason for hiding this comment

yamt commented Jan 20, 2023

loganek commented Jan 20, 2023

loganek Jan 6, 2023 •

edited

Loading