-
Notifications
You must be signed in to change notification settings - Fork 25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement a simple test filter #35
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
{ | ||
"WASI C tests": { | ||
"stat-dev-ino": "d_ino is known broken in this combination of engine and wasi-sdk version." | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -62,8 +62,12 @@ def test_runner_end_to_end() -> None: | |
|
||
reporters = [Mock(), Mock()] | ||
|
||
filt = Mock() | ||
filt.should_skip.return_value = (False, None) | ||
filters = [filt] | ||
|
||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we validate if filters actually were called? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it'd be also good to validate the case when the filter returns |
||
|
||
# Assert manifest was read correctly | ||
assert suite.name == "test-suite" | ||
|
@@ -91,9 +95,15 @@ def test_runner_end_to_end() -> None: | |
for config, output in zip(expected_config, outputs): | ||
validator.assert_any_call(config, output) | ||
|
||
# Assert filter calls | ||
for filt in filters: | ||
assert filt.should_skip.call_count == 3 | ||
for test_case in expected_test_cases: | ||
filt.should_skip.assert_any_call(suite.name, test_case.name) | ||
|
||
|
||
@patch("os.path.exists", Mock(return_value=False)) | ||
def test_runner_should_use_path_for_name_if_manifest_does_not_exist() -> None: | ||
suite = tsr.run_tests_from_test_suite("my-path", Mock(), [], []) | ||
suite = tsr.run_tests_from_test_suite("my-path", Mock(), [], [], []) | ||
|
||
assert suite.name == "my-path" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
from typing import Tuple, Union, Literal | ||
from abc import ABC | ||
from abc import abstractmethod | ||
|
||
import json | ||
|
||
|
||
class TestFilter(ABC): | ||
@abstractmethod | ||
def should_skip( | ||
self, test_suite_name: str, test_name: str | ||
) -> Union[Tuple[Literal[True], str], Tuple[Literal[False], Literal[None]]]: | ||
pass | ||
|
||
|
||
class JSONTestExcludeFilter(TestFilter): | ||
def __init__(self, filename: str) -> None: | ||
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 | ||
) -> Union[Tuple[Literal[True], str], Tuple[Literal[False], Literal[None]]]: | ||
test_suite_filter = self.filter_dict.get(test_suite_name) | ||
if test_suite_filter is None: | ||
return False, None | ||
why = test_suite_filter.get(test_name) | ||
if why is not None: | ||
return True, why | ||
return False, None |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,7 @@ | |
from datetime import datetime | ||
from typing import List, cast | ||
|
||
from .filters import TestFilter | ||
from .runtime_adapter import RuntimeAdapter | ||
from .test_case import ( | ||
Result, | ||
|
@@ -25,28 +26,52 @@ def run_tests_from_test_suite( | |
runtime: RuntimeAdapter, | ||
validators: List[Validator], | ||
reporters: List[TestReporter], | ||
filters: List[TestFilter], | ||
) -> TestSuite: | ||
test_cases: List[TestCase] = [] | ||
test_start = datetime.now() | ||
|
||
_cleanup_test_output(test_suite_path) | ||
|
||
test_suite_name = _read_manifest(test_suite_path) | ||
|
||
for test_path in glob.glob(os.path.join(test_suite_path, "*.wasm")): | ||
test_case = _execute_single_test(runtime, validators, test_path) | ||
test_name = os.path.splitext(os.path.basename(test_path))[0] | ||
for filt in filters: | ||
# for now, just drop the skip reason string. it might be | ||
# useful to make reporters report it. | ||
skip, _ = filt.should_skip(test_suite_name, test_name) | ||
if skip: | ||
test_case = _skip_single_test(runtime, validators, test_path) | ||
break | ||
else: | ||
test_case = _execute_single_test(runtime, validators, test_path) | ||
test_cases.append(test_case) | ||
for reporter in reporters: | ||
reporter.report_test(test_case) | ||
|
||
elapsed = (datetime.now() - test_start).total_seconds() | ||
|
||
return TestSuite( | ||
name=_read_manifest(test_suite_path), | ||
name=test_suite_name, | ||
time=test_start, | ||
duration_s=elapsed, | ||
test_cases=test_cases, | ||
) | ||
|
||
|
||
def _skip_single_test( | ||
_runtime: RuntimeAdapter, _validators: List[Validator], test_path: str | ||
) -> TestCase: | ||
config = _read_test_config(test_path) | ||
return TestCase( | ||
name=os.path.splitext(os.path.basename(test_path))[0], | ||
config=config, | ||
result=Result(output=Output(0, "", ""), is_executed=False, failures=[]), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe. i'd like to postpone it for later PRs. it would be nicer to be able to represent "timed out" as well. #42 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alright, we can refactor that piece as part of #42 then. |
||
duration_s=0, | ||
) | ||
|
||
|
||
def _execute_single_test( | ||
runtime: RuntimeAdapter, validators: List[Validator], test_path: str | ||
) -> TestCase: | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if we actually need such a detailed filters for now. Initially I was thinking of the following requirements:
--filter-include ".*thread.*" --filter-exclude ".*open.*"
)--filter-exclude "WASI C tests.*"
)But I'm curious what are your thoughts on that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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):
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my suggestion for this PR:
--filter
option to--exclude-filter
--include-filter
,--exclude-regex-filter
etc later when/if necessary.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
given the current cli options which allows to specify multiple testsuites,
i thought it's natural to accept multiple filters.
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:
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To unblock the change, let's move on with your suggestion here #35 (comment), we'll adapt it once we have more feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done