-
Notifications
You must be signed in to change notification settings - Fork 1
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
[FR] Add support for configurable tests and validation #4
[FR] Add support for configurable tests and validation #4
Conversation
custom-rules config files for testing |
clear_caches() | ||
ctx.exit(pytest.main(["-v"])) | ||
ctx.exit(pytest.main(['-v'] + tests)) |
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 initially built a custom class around unittest.TestLoader
, but since we build tests in unittest and call via pytest (for formatting), it was moot and easier to just pass as a command line arg
It's worth noting that this POC supports a feature that pytest doesn't support natively which is the ability to explicitly list test or a regex of tests I also like that we're not touching as many individual unit tests files. |
Since we call via pytest, we can use conftest for discovery and enumeration and use the builtin fixture. You basically create a # conftest.py
import pytest
def pytest_collection_modifyitems(config, items):
for testcase in items:
# Print test names
print(f"Collected test: {testcase.name} with full path {testcase.nodeid}")
# Skip tests marked with 'elastic' mark if we did decorate with pytest markers
if testcase.get_closest_marker("elastic"):
testcase.add_marker(pytest.mark.skip(reason="Skipping test marked with 'elastic'."))
# Skip tests based on test config
... You could also skip by regex if needed: if re.match(pattern_to_skip, testcase.nodeid):
testcase.add_marker(pytest.mark.skip(reason="Test name matches skip pattern.")) |
@@ -32,4 +31,12 @@ files: | |||
# | |||
|
|||
# testing: | |||
# TBD... | |||
# config: etc/example_test_config.yaml |
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.
may need to specify a custom test path
|
||
|
||
@dataclass | ||
class TestConfig: |
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'd recommend moving this class to a dedicated module since it seems like it has a mode dedicated purpose than miscellaneous methods/classes.
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.
++ totes, I'll make a config.py
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.
and move all of this
elif self.unit_tests.bypass: | ||
tests = [] | ||
skipped = [] | ||
for test in self.all_tests: | ||
if test not in combined_tests: | ||
tests.append(test) | ||
else: | ||
skipped.append(test) |
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 think with a few set / list operations we can condense this.
basically all_tests - skipped should be tests right?
def fmt_tests(lt) -> List[str]: | ||
raw = [t.rsplit('.', maxsplit=2) for t in lt] | ||
ft = [] | ||
for test in raw: | ||
path, clazz, method = test | ||
path = f'{path.replace(".", os.path.sep)}.py' | ||
ft.append('::'.join([path, clazz, method])) | ||
return ft | ||
|
||
return fmt_tests(tests), fmt_tests(skipped) | ||
else: | ||
return tests, skipped |
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.
can we break out this to a staticmethod of the class w/doc strings, renamed function arg, + doc string etc.
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.
good idea 👍
if not (bypass or test_only): | ||
return False | ||
return (bypass and rule_id in bypass) or (test_only and rule_id not in test_only) |
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.
For readability, do we want to expand/refactor this?
e.g.
if self.rule_validation.bypass and rule_id in self.rule_validation.bypass:
return True
if self.rule_validation.test_only and rule_id not in self.rule_validation.test_only:
return True
return False
or even, close to as-is
bypass_tests = self.rule_validation.bypass
test_only_tests = self.rule_validation.test_only
if bypass_tests and rule_id in bypass_tests:
return True
if test_only_tests and rule_id not in test_only_tests:
return True
return False
def parse_out_patterns(names: List[str]) -> (List[str], List[str]): | ||
"""Parse out test patterns from a list of test names.""" | ||
patterns = [] | ||
tests = [] | ||
for name in names: | ||
if name.startswith('pattern:') and '*' in name: | ||
patterns.append(name[len('pattern:'):]) | ||
else: | ||
tests.append(name) | ||
return patterns, tests |
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.
If we don't prefix lines in the config with pattern
, I think fnmatch
will still work as expected. Do we need to prefix and parse?
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.
🤔 that should work, yea. Since full name would have a single match only. I can't remember if there was any other reason why I broke them out though. If not, I'll simplify it with this
@brokensound77 we have things in our repo to bypass schema validation (e.g. |
…ions [FR] Add support to decouple actions and exceptions
Updates for configurable tests. This is just to show the delta since this targets my custom directory support PR.
Summary
This makes unit tests configurable by name and pattern and rule validation by rule_id, configurable by a
test_config,yaml
file.Details
The test file is exposed either by setting
DETECTION_RULES_TEST_CONFIG
to the path, or by referencing it from within the_config.yaml
. The enivronment variable allow bypassing tests on prebuilt rules where modifying the built-in_config.yaml
is not feasible due to VCS.Opting in/out of tests
Opting out of rule validation
Testing
Sample config:
1.
make test
(orpython -m detection_rules test
)2.
DETECTION_RULES_TEST_CONFIG=path/to/test_config.yaml make test
3.
CUSTOM_RULES_DIR=custom-rules make test
testing.config
set toetc/test_config.yaml
A config directory is attached for convenience