Opened 13 months ago

Closed 13 months ago

Last modified 10 months ago

#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
https://codereview.adblockplus.org/29941688/

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.

Change History (17)

comment:1 in reply to: ↑ description ; follow-up: Changed 13 months ago by sebastian

  • Cc mjethani sebastian added

Replying to erikvold:

also the ? in .*? is redundant.

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.

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.

comment:2 Changed 13 months ago by erikvold

  • Description modified (diff)

comment:3 in reply to: ↑ 1 Changed 13 months ago 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).
Last edited 13 months ago by erikvold (previous) (diff)

comment:4 Changed 13 months ago 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 13 months ago by sebastian

  • Blocking 6833 removed

comment:6 Changed 13 months ago 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:7 Changed 13 months ago by ukacar

  • Review URL(s) modified (diff)

comment:8 Changed 13 months ago by erikvold

  • Type changed from defect to change

comment:9 Changed 13 months ago by abpbot

A commit referencing this issue has landed:
Issue 7086 - Added more tests for handling special comments

comment:10 Changed 13 months ago by kzar

  • Cc kzar added
  • Description modified (diff)
  • Priority changed from Unknown to P2
  • Ready set

comment:11 Changed 13 months ago by ukacar

  • Owner set to ukacar

comment:12 Changed 13 months ago by ukacar

  • Status changed from new to reviewing

comment:13 Changed 13 months ago by ukacar

  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:14 Changed 13 months ago by hfiguiere

  • Resolution fixed deleted
  • Review URL(s) modified (diff)
  • Status changed from closed to reopened

comment:15 Changed 13 months ago by abpbot

A commit referencing this issue has landed:
Issue 7086 - Fix typo / linting error

comment:16 Changed 13 months ago by hfiguiere

  • Resolution set to fixed
  • Status changed from reopened to closed
Note: See TracTickets for help on using tickets.