Don't use old method in tests #560
Conversation
![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: 82d4cce | Previous: 1e93800 | Ratio |
|---|---|---|---|
rule-match-browserlike/brave-list |
1919195927 ns/iter (± 5761544) |
2051836000 ns/iter (± 9414636) |
0.94 |
rule-match-first-request/brave-list |
1155935 ns/iter (± 9498) |
1112038 ns/iter (± 6293) |
1.04 |
blocker_new/brave-list |
163477165 ns/iter (± 777263) |
143433250 ns/iter (± 539931) |
1.14 |
blocker_new/brave-list-deserialize |
24306380 ns/iter (± 466997) |
23670075 ns/iter (± 229117) |
1.03 |
memory-usage/brave-list-initial |
10213328 ns/iter (± 3) |
10213344 ns/iter (± 3) |
1.00 |
memory-usage/brave-list-initial/max |
60612203 ns/iter (± 3) |
60612235 ns/iter (± 3) |
1.00 |
memory-usage/brave-list-initial/alloc-count |
1231663 ns/iter (± 3) |
1231711 ns/iter (± 3) |
1.00 |
memory-usage/brave-list-1000-requests |
2692648 ns/iter (± 3) |
2692712 ns/iter (± 3) |
1.00 |
memory-usage/brave-list-1000-requests/alloc-count |
71527 ns/iter (± 3) |
71607 ns/iter (± 3) |
1.00 |
url_cosmetic_resources/brave-list |
195617 ns/iter (± 1933) |
191829 ns/iter (± 505) |
1.02 |
cosmetic-class-id-match/brave-list |
3636081 ns/iter (± 981615) |
3415862 ns/iter (± 942750) |
1.06 |
This comment was automatically generated by workflow using github-action-benchmark.
| .compiled_regex_count, | ||
| 0 | ||
| ); | ||
| // Regex can't be compiled, so no match. |
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
src/filters/network.rs
Outdated
| } | ||
|
|
||
| #[doc(hidden)] | ||
| pub fn matches_test(&self, request: &request::Request) -> bool { |
There was a problem hiding this comment.
matches_test was used tests/matching.rs for exception checking.
I've adjusted the tests to use directly engine.check_network_request and added #[cfg(test)]
tests/unit/regex_manager.rs
Outdated
| let mut regex_manager = RegexManager::default(); | ||
| regex_manager.update_time(); | ||
| let engine = make_engine("||geo*.hltv.org^"); | ||
| engine.borrow_regex_manager(); |
There was a problem hiding this comment.
comment would also be useful here to explain why the extra borrows are necessary
There was a problem hiding this comment.
Actually, we don't need this with thread-local mock time. Removed.
| #[cfg(test)] | ||
| pub fn borrow_regex_manager(&self) -> crate::blocker::RegexManagerRef<'_> { |
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)
There was a problem hiding this comment.
It's just a test wrapper for blocker.borrow_regex_manager, so I added a doc comment to borrow_regex_manager() there.
| impl NetworkMatchable for NetworkFilter { | ||
| fn matches(&self, request: &request::Request, regex_manager: &mut RegexManager) -> bool { |
There was a problem hiding this comment.
fwiw, this is a breaking API change and will require another bump up to 0.12.x
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Rust Benchmark'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.10.
| Benchmark suite | Current: 82d4cce | Previous: 1e93800 | Ratio |
|---|---|---|---|
blocker_new/brave-list |
163477165 ns/iter (± 777263) |
143433250 ns/iter (± 539931) |
1.14 |
This comment was automatically generated by workflow using github-action-benchmark.
|
merged to |
The PR removes
impl NetworkMatchable for NetworkFilterand port the related tests to the flatbuffer impl (that we actually use in production)