Add event matching test suite #10599
Conversation
22b67cf to
bb6614a
Compare
LocalStack Community integration with Pro 2 files 2 suites 1h 31m 18s ⏱️ Results for commit bb6614a. |
| # and exception handling. | ||
| @pytest.mark.skipif(is_v2_provider(), reason="V2 provider does not support this feature yet") | ||
| @pytest.mark.parametrize( | ||
| "request_template,label", request_template_tuples, ids=[t[1] for t in request_template_tuples] |
There was a problem hiding this comment.
probably debatable whether it's a good idea to parameterize tests based on directory contents. It makes it very easy to add new test cases but add a small delay to test collection because it needs to list the files in the directory 🤷
There was a problem hiding this comment.
I think this is fine for tests like this :)
| @@ -261,7 +263,8 @@ def test_put_event_with_content_base_rule_in_pattern(aws_client, clean_up): | |||
| "test2": [{"anything-but": "test2"}], | |||
| "test3": [{"anything-but": ["test3", "test33"]}], | |||
| "test4": [{"anything-but": {"prefix": "test4"}}], | |||
| "ip": [{"cidr": "10.102.1.0/24"}], | |||
There was a problem hiding this comment.
That IP check never worked but it got silently ignored 😬 (just double-checked by running the test suite against the old implementations before the refactorings and fixes)
| # and exception handling. | ||
| @pytest.mark.skipif(is_v2_provider(), reason="V2 provider does not support this feature yet") | ||
| @pytest.mark.parametrize( | ||
| "request_template,label", request_template_tuples, ids=[t[1] for t in request_template_tuples] |
There was a problem hiding this comment.
I think this is fine for tests like this :)
| event = request_template["Event"] | ||
| event_pattern = request_template["EventPattern"] | ||
|
|
||
| if label.endswith("_EXC"): |
There was a problem hiding this comment.
What was your reasoning here for encoding this in the file name, instead of having another field in the json5 file?
There was a problem hiding this comment.
Good question. It kinda evolved because I wanted to have an explicit way to express the intent of the test. The surprise effect helps to write better test cases. It could as well be a field in the the json5 file though.
Not really a strong reason, some thoughts:
- The intention of the test is easy to see in the file explorer.
- It currently matches the TestEventPattern API. Maybe, it would be easier to copy/paste and test in the AWS console.
There was a problem hiding this comment.
I initially built a small templating engine supporting variables but noticed we can simplify and don't care about any dynamic values 🤦 So the entire request "templating" part comes from there 😅
| # EventBridge apparently converts some fields, for example: Source=>source, DetailType=>detail-type | ||
| # but the actual pattern matching is case-sensitive by key! |
| ) | ||
|
|
||
| # Validate the test intention: The _NEG suffix indicates negative tests (i.e., a pattern not matching the event) | ||
| if label.endswith("_NEG"): |
There was a problem hiding this comment.
It might be useful in the future to snapshot the result together with a hash of the request because currently, it's possible to modify the test case after being AWS-validated; kinda proper testing police ;)
Motivation
Event filtering is insufficiently tested and implemented in too many places in LocalStack 😢
Before we can unify all existing implementations, we need a robust AWS-validated test suite.
Existing implementations in LocalStack:
EventPatternmodels of moto-ext implements the same Amazon EventBridge event patterns in ~50 LOC. The implementation is short and concise (e.g., mapping operators to functions<=>lt) but incomplete. LocalStack patches this implementation in the EventBridge provider. Hence, it is not used within LocalStack.AWS recently (2024-02-01) open-sourced https://github.com/aws/event-ruler. This rule engine implements the above filtering patterns. It is written in Java in ~6k LOC. Their blog post mentions that the internals are built on top of finite state machines, which can be updated dynamically with new rules.
Changes
test_event_patternin the event provider use our EventBridge filtering implementationfilter_event=>matches_event) and fix several issues in the EventBridgefilter_eventsmethod and its helpers (e.g., improved handling of numeric operators and improved list intersections handling)filter_stream_record=>does_match_event) and fix several issues (e.g., operator casing, numeric conditions, raising exceptions)Testing
Within localstack/services/events/provider.py,
test_event_patterncan be used to invoke different implementation within LocalStack.Discussion
Raising exceptions instead of silently ignoring the error case and returning True/False is a behavior change that can help us to make issues more visible. Background processes should handle such exceptions better (e.g., The SQS ESM implementation just drops the entire batch upon unhandled exceptions). At AWS, more exceptions are probably caught earlier because the rule engine can validate rules before actually doing a matching.