Don't use old method in tests #560
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Rust Benchmark
| Benchmark suite | Current: 9102886 | Previous: e9299eb | Ratio |
|---|---|---|---|
rule-match-browserlike/brave-list |
1778782824 ns/iter (± 2335270) |
1972094868 ns/iter (± 5118480) |
0.90 |
rule-match-first-request/brave-list |
1125346 ns/iter (± 5335) |
1114948 ns/iter (± 7936) |
1.01 |
blocker_new/brave-list |
152508811 ns/iter (± 1250572) |
149920984 ns/iter (± 359157) |
1.02 |
blocker_new/brave-list-deserialize |
20873957 ns/iter (± 116935) |
26447852 ns/iter (± 105485) |
0.79 |
memory-usage/brave-list-initial |
10212216 ns/iter (± 3) |
10212216 ns/iter (± 3) |
1 |
memory-usage/brave-list-initial/max |
63149135 ns/iter (± 3) |
63149135 ns/iter (± 3) |
1 |
memory-usage/brave-list-initial/alloc-count |
1231721 ns/iter (± 3) |
1231721 ns/iter (± 3) |
1 |
memory-usage/brave-list-1000-requests |
2645922 ns/iter (± 3) |
2645938 ns/iter (± 3) |
1.00 |
memory-usage/brave-list-1000-requests/alloc-count |
70984 ns/iter (± 3) |
71016 ns/iter (± 3) |
1.00 |
url_cosmetic_resources/brave-list |
186713 ns/iter (± 553) |
189954 ns/iter (± 1211) |
0.98 |
cosmetic-class-id-match/brave-list |
3502960 ns/iter (± 935804) |
3389120 ns/iter (± 946890) |
1.03 |
This comment was automatically generated by workflow using github-action-benchmark.
atuchin-m
commented
Nov 11, 2025
| .compiled_regex_count, | ||
| 0 | ||
| ); | ||
| // Regex can't be compiled, so no match. |
Collaborator
Author
There was a problem hiding this comment.
the test case added here (already in the disabled state):
40b7815
regex crate can't parse such regex, so I changed the test to verify it doesn't match
atuchin-m
force-pushed
the
pr-dont-use-old-methods-in-tests
branch
4 times, most recently
from
1c5c5ec to
fceebb0
Compare
atuchin-m
force-pushed
the
pr-dont-use-old-methods-in-tests
branch
from
fceebb0 to
9102886
Compare
antonok-edm
reviewed
Nov 14, 2025
Comment on lines
+158
to
+159
| #[doc(hidden)] | ||
| pub(crate) fn check_exceptions(&self, request: &Request) -> bool { |
Collaborator
There was a problem hiding this comment.
no need to include #[doc(hidden)] on pub(crate) items
| } | ||
|
|
||
| #[doc(hidden)] | ||
| pub fn matches_test(&self, request: &request::Request) -> bool { |
| let mut regex_manager = RegexManager::default(); | ||
| regex_manager.update_time(); | ||
| let engine = make_engine("||geo*.hltv.org^"); | ||
| engine.borrow_regex_manager(); |
Collaborator
There was a problem hiding this comment.
comment would also be useful here to explain why the extra borrows are necessary
Comment on lines
+274
to
+275
| #[cfg(test)] | ||
| pub fn borrow_regex_manager(&self) -> crate::blocker::RegexManagerRef<'_> { |
Collaborator
There was a problem hiding this comment.
comment describing why this is necessary would be helpful
(I can tell from the diffs below, but later it won't be as obvious)
Comment on lines
-996
to
-997
| impl NetworkMatchable for NetworkFilter { | ||
| fn matches(&self, request: &request::Request, regex_manager: &mut RegexManager) -> bool { |
Collaborator
There was a problem hiding this comment.
fwiw, this is a breaking API change and will require another bump up to 0.12.x
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The PR removes
impl NetworkMatchable for NetworkFilterand port the related tests to the flatbuffer impl (that we actually use in production)