Opened on 10/31/2018 at 01:59:02 AM
Closed on 11/09/2018 at 10:55:38 PM
Last modified on 02/07/2019 at 03:23:52 AM
#7086 closed change (fixed)
Add tests for handling unknown special comments, and comments mixed with special comments
| Reported by: | erikvold | Assignee: | ukacar |
|---|---|---|---|
| Priority: | P2 | Milestone: | |
| Module: | Core | Keywords: | |
| Cc: | mjethani, sebastian, kzar | Blocked By: | |
| Blocking: | Platform: | Unknown / Cross platform | |
| Ready: | yes | Confidential: | no |
| Tester: | Unknown | Verified working: | no |
| Review URL(s): |
https://gitlab.com/eyeo/adblockplus/adblockpluscore/merge_requests/4 |
||
Description (last modified by kzar)
It seems like how to handle unknown special comments was never defined somehow.
In this commit for #6923 there was a change made to a regular expression. By changing a \w+ bit for a .*? bit, for which no tests were added.
The change adds support for special comments like ! : blah or ! *** : foo by simply ignoring it, this comment previously would've been treated as a filter rule and any special comments after it would've been treated as a filter rule too.
Let's at least add some unit tests for this.
Attachments (0)
Change History (17)
comment:1 in reply to: ↑ description ; follow-up: ↓ 3 Changed on 10/31/2018 at 02:21:39 AM by sebastian
- Cc mjethani sebastian added
comment:3 in reply to: ↑ 1 Changed on 10/31/2018 at 03:04:03 AM by erikvold
Replying to sebastian:
Replying to erikvold:
also the ? in .*? is redundant.
It is not, it makes the token non-greedy.
Ah yes, my mistake, sorry!
Also, I'm wondering why the change was made? because if this is a use case that we want to support then I'm not sure why we would want to support the use case. If it's not then we should also have a test that we do not support this use case.
See this announcement for why we started to require metadata to be given at the top of the filter list, see my comment here explaining why we have to account for invalid metadata keys as well.
I see that helps thanks.
So it sounds to me based on above and discussion in #6923, we want:
- to support/allow unknown special comments like ! :, !!@#$%^&*() : , and ! Some Unknown Comment : (we need some tests for this).
- !foo mixed in with special comments should not be allowed, special comments must precede regular comments and filters (we need to add tests for this).
comment:4 Changed on 10/31/2018 at 03:30:27 AM by erikvold
- Summary changed from Handling unknown special comments to Add tests for handling unknown special comments, and comments mixed with special comments
comment:5 Changed on 10/31/2018 at 10:05:13 PM by sebastian
- Blocking 6833 removed
comment:6 Changed on 11/04/2018 at 01:49:32 AM by mjethani
Could you change this from "defect" to "change" and update the description now based on the above discussion? It looks like we need some tests, am I right? Let's then say that in the description.
comment:8 Changed on 11/09/2018 at 08:18:37 PM by erikvold
- Type changed from defect to change
comment:9 Changed on 11/09/2018 at 10:23:22 PM by abpbot
A commit referencing this issue has landed:
Issue 7086 - Added more tests for handling special comments
comment:10 Changed on 11/09/2018 at 10:33:56 PM by kzar
- Cc kzar added
- Description modified (diff)
- Priority changed from Unknown to P2
- Ready set
comment:11 Changed on 11/09/2018 at 10:34:39 PM by ukacar
- Owner set to ukacar
comment:12 Changed on 11/09/2018 at 10:35:24 PM by ukacar
- Status changed from new to reviewing
comment:13 Changed on 11/09/2018 at 10:36:08 PM by ukacar
- Resolution set to fixed
- Status changed from reviewing to closed
comment:14 Changed on 11/09/2018 at 10:53:18 PM by hfiguiere
- Resolution fixed deleted
- Review URL(s) modified (diff)
- Status changed from closed to reopened
comment:15 Changed on 11/09/2018 at 10:54:57 PM by abpbot
A commit referencing this issue has landed:
Issue 7086 - Fix typo / linting error
comment:16 Changed on 11/09/2018 at 10:55:38 PM by hfiguiere
- Resolution set to fixed
- Status changed from reopened to closed
comment:17 Changed on 02/07/2019 at 03:23:52 AM by abpbot
Some commits referencing this issue have landed:
Replying to erikvold:
It is not, it makes the token non-greedy. Otherwise, for example in ! Homepage: https://example.com the key would be Homepage: https and the value //example.com.
See this announcement for why we started to require metadata to be given at the top of the filter list, see my comment here explaining why we have to account for invalid metadata keys as well.