The existing test case for the filter module is mainly testing the administration interface, and not the individual filters themselves. Here's a preliminary patch that adds some tests for the URL filter based on some edge cases from #161217: URL filter breaks generated href tags. Eventually, every filter should get tested individually.

CommentFileSizeAuthor
#8 filter.test-249200-8.patch16.31 KBfloretan
#6 filter.test-0.3.patch16.13 KBhingo
#4 filter.test-249200-4.patch15.93 KBfloretan
#3 filter.test-0.2.patch15.74 KBhingo
#2 filter.test-0.2.patch15.74 KBhingo
filter.test.patch10.21 KBfloretan

Comments

floretan’s picture

floretan commented
Status: Active » Needs work
hingo’s picture

hingo commented
Status: Needs work » Needs review
StatusFileSize
new filter.test-0.2.patch15.74 KB

Hi flobruit

Thanks for paying attention to this.

I've taken your patch as base, but actually changed the approach a little bit. The point is that some bugs cannot be found by testing just simple strings one by one. For instance, a typical error I made myself in some rgexps would be that urlfilter converts only the first URL in a paragraph but not the subsequent ones. For such reasons I believe it is better to test against one big data sample than several small ones.

So my approach is to have only one big sample data file (using heredoc syntax, I hope that is acceptable?). It contains several url's of the form www.test1.com www.test2.com ... Then there are 42 asserts that checks each of them. (The current url filter fails 12 tests, my patch passes all.)

If you approve of the general approach, I'd say this patch is ready to be committed.

Note also that this patch is against a newer version of Drupal HEAD than yours.

hingo’s picture

hingo commented
StatusFileSize
new filter.test-0.2.patch15.74 KB

Ah, the previous patch has the patch the wrong way (it will take you back from the new test to the original). This is with the diff orgfile newfile the right way.

floretan’s picture

floretan commented
StatusFileSize
new filter.test-249200-4.patch15.93 KB

heredoc notation is probably good for this, and having one chunk of text makes more sense than a lot of little ones. I would clean up the text a little bit more, like breaking those 100+ character lines into multiple lines. I would also document in the sample text what is being tested, for example:

<!-- URL's in HTML comments should not be parsed: http://www.test29.com -->

Note to self: patches created with diff instead of cvs diff can be applied from the command-line, but not from Eclipse.

hingo’s picture

hingo commented
Status: Needs review » Needs work

I would clean up the text a little bit more, like breaking those 100+ character lines into multiple lines.

Actually, there shouldn't be any linebreaks added apart from the ones that already exist. Linebreaks may impact how regexps or parsing are done, so the long paragraphs in this sample text are there on purpose.

I would also document in the sample text what is being tested:

This is a good idea. I hope I will have some more time during the weekend (I'm father of a small child, hacking time is scarce nowadays!) and get back to you.

hingo’s picture

hingo commented
Status: Needs work » Needs review
StatusFileSize
new filter.test-0.3.patch16.13 KB

* Reformatted test data to be self commenting
* Added a test for ftp:// url
* Code is ready for review and eventual commit

dries’s picture

dries commented

This looks like a fine patch. I'd rename 'http' to 'HTTP' and 'html' to 'HTML' when used as words/acronyms. Great work.

floretan’s picture

floretan commented
StatusFileSize
new filter.test-249200-8.patch16.31 KB

Updated patch according to the recommendation in #7.

dries’s picture

dries commented
Status: Needs review » Fixed

Committed to CVS HEAD.

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Title: Test individual filters. » Filter tests fail
Category: task » bug
Priority: Normal » Critical
Status: Closed (fixed) » Active

Hm. Did this patch ever work?

I get failures running the filter tests that were added by this patch in HEAD, but even when I revert back to this specific commit, they still don't pass. I had Bdragon on IRC confirm.

Are these failing because of legitimate bugs, or because the tests are wrong?

boombatower’s picture

boombatower commented
Component: filter.module » tests

Change component is relation to http://drupal.org/node/253744.

hingo’s picture

hingo commented

@Webchick

No, these tests always failed, because the current URL filter is not very good. The point of writing these tests was to clearly point out simple cases that the current URL filter does wrongly (such as mangling perfectly valid <a href="..." title="...">links</a> written manually by the poster.

There is a bug about this that has been open for almost a year now with a patch that passes all these tests in #161217: URL filter breaks generated href tags

(Can we put this back to fixed/closed? The tests are ok it is the filter that is buggy.)

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Title: Filter tests fail » Test individual filters.
Status: Active » Fixed

Yes, that's great! Thank you.

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.