Regression: Filter tests fail

See http://drupal.org/node/251225#comment-848930

Since filter.module had this bug for ages, ideally the test would've been rolled into the patch at #161217: URL filter breaks generated href tags rather than committed separately.

Since that patch still has some work needs doing on it, in the interests of getting a baseline of 100% test passes, we could consider rolling back the URL filter test, merging it into that issue, and committing it again along with the bug fix. Here's a patch for that.

I am for that.

Test passes.

As per #276267: Fix the minimum cache lifetime feature same reason.

Can we just comment this out until the patch gets in?

ping.

I really do not like committing this patch, but I did it anyway.

So you mean since the incumbent URL filter fails so many test cases, the right fix is to just remove the tests! This really boosted my confidence in url filter ever being fixed...

No, the right fix is to attach the tests to the actual issue where the URL filter is being worked on, which is what I've just done, and what should have happened in the first place.

There are 950 active bugs against 7.x and 6.x at the moment - do you think we should add tests for those without appropriate fixes? When you're sitting in irc being asked whether x test failure is a known issue or not every five minutes you might have a different answer.

http://drupal.org/node/161217#comment-965344 needs review.

It's a matter of opinion really. In the real world (which is, companies I worked in) it is quite common to have tests failing first, then working towards 100% (and often releasing before getting there). And they have processes to deal with such a situation. Typically tests should work and be documented in such a way that when tests fail, you should know whether they have anything to do with your code or not. (So for instance, tests should be grouped by *.module file, not just one big "core" module.)

Sure it is convenient to always have all tests pass. In some ways it is also a good logistical approach to also commit new tests and code in one patch. OTOH having the same person writing both the tests and the code isn't always that good either. A stupid developer will write stupid tests accompanied with buggy code, tests still pass 100%. He will even break working code like that. It's not bulletproof you know. (proof below)

While maintaining my own module, I'm not that active in the Drupal community. This is the first (and regrettably also last) time I've posted a patch against core. The biggest revelation here has therefore been the share laziness and lack of discipline that even Drupal core developers seem to have. For instance in this case a patch that fixes the failing tests has existed much longer than the actual tests. You even know about it because you yourself already asked once when the tests were committed. But you never cared to engage with the patch that fixes it for 2 months now. Then eventually you see it as your right to remove the tests from core instead. You could have chosen to apply the patch with the same time and effort, but chose not to. Dries is absolutely right when he says that you are now optimizing for tests passing rather than doing anything to improve Drupal.

Sure, someone might claim that the patch still needs work, it may be confusing to know the situation since lately there's been a flood of people who come by and post broken editions of it. In reality nobody has found any bug or other issue (apart from whitespace or punctuation issues) since nearly a year now. It's a trivial patch and just reading the thread would reveal this is true, but nobody bothers. To further prove my point, go and see how easily catch was lured into now also breaking the tests. Do they even pass anymore? I bet not and even if they do the results are nonsensical, they don't actually test anything anymore. So now you have not only broken my patch several times, you also have broken the tests. Focus people, focus.

So as not to make this look like a complete rant, I'll end by some statistics so you can all be ashamed of yourself:

July 22 - Sep 25 2007 (2 months)
Creating and finalizing the patch:
* Post initial patch
* Fix diff format to the one used by Drupal
* "Steven" makes pretty much the only sane comment in the whole thread until now, which I eventually implement
* Fix style issues
* First erroneous review appears, but that is ok, you can easily be fooled by the cache
* This leads me to actually find a remaining bug, which is fixed by rewriting to Steven's approach
* Fixing patch to use UTF8
* NOTE: after this date there are no bugs found nor functional improvements done to the patch!

Oct 6 - Mar 5 2008 (5 months)
Nothing happens
* People affected by the bug start asking why nothing is happening

Mar 5 - Apr 20 (1,5 months)
First code style comments appear
* Starting a problematic pattern, see my comments below

Apr 21 - May 5 (2 weeks)
Tests are committed to core, patch is not
* flobruit shows some cunning thinking in piggybacking on the effort to create a test framework to draw attention to filter.module being buggy. Tests are quickly committed to core, patch not so much. But it did raise awareness of the issue.

May 5 - June 10 (1 month)
The so called "peer review" kicks in again, with more style issues
* Remember we already had some code style discussion before. By now I noticed a common pattern: people pop by, "helpfully" pointing out 1-2 issues (like "remove whitespace here") and switching status to CNW. Then when the issue is fixed, they return with another issue ("capitalize this").
* The right way would be to focus a little and read the whole patch through and list all issues in one review. Or if uncapable of this, shut up, you're doing more harm than good. (This is like unix philosophy: focus on doing one thing and do it well.)
* One of the reviewers here especially makes a fool of himself by proposing edits to code comments that clearly show he doesn't even understand the code he is reviewing, thus proposing nonsensical comments.

June 10 - July 10 (1 month)
Others start writing patches, which are mostly broken
* Seeing that I've become grumpy, more helpful people start actually doing code style edits into the patch, instead of commenting to me. Kind of the right way to go, except they introduce bugs in the code while editing whitespace.
* Favorite quote: "my patch removed the whole if ($i % 2 == 0) business because I found it ugly". Sure, but the code also had a function, you can't just remove something because it hurts your eyes, now your patch is buggy.
* I try to play along, rolling back to my latest working patch.
* I even try to appease my "reviewers" by implementing a small restructuring of code that gets rid of some level of indenting. No functional changes though. As I said, patch was ready in Sep 2007 and hasn't changed (for the better) since.

July 10 - Aug 15 (1 month)
I give up
* I announce that I've committed the new url filter into footnotes.module
* filter.module tests are removed from core
* I announce that I completely give up

During the lifetime (so far!) of the patch there has been

6 code style comments or patches by 5 separate people
...out of which esp. 1 makes it clear that commenter didn't even understand what he is commenting so even if he is not commenting on code he gets it wrong!
3 code refactoring patches by 3 separate people
...out of which 3 introduced bugs to a bug free patch
...out of which 2 are shown as a PHP notice *each* time the filter is run, indicating that person posting the patch didn't test at all before posting it (just make a test post would have revealed it)
...and 1 makes Drupal not run at all (PHP syntax error) indicating that person didn't test at all before posting
1 person claims to fix a bug but is actually introducing it. (Confusion on simple arithmetics here.)
1 post (that is not me) reverts to an older working patch to fix errors done by others (I've obviously done it several times)
1 comment on the newly introduced filter.test code that is erroneous, not understanding what you are commenting on
1 person instantly posting a patch implementing the erroneous suggestion

Contrast with

1 comment turned out to be helpful, eventually fixing a bug (Steven)
1 bogus comment that lead me to find a bug anyway
2 correct comments that I should fix patch format or encoding
1 helpful person has an idea how to get the patch committed (test cases), but our effort fails in the end
0 patches by others than me that fix anything
0 comments that find and point to a real bug

...and as said, nothing helpful at all happened after Sep 25, 2007.

That is all well and good, I share you frustration as I have waited 4 months to get a patch into core. The process is somewhat annoying and at times very slow.

There are different ways to do everything and I definitely agree that having failing tests is one way to go. I don't, however, agree that it is best for large developer communities. I already get tons of extraneous questions about what tests pass/fail. If we start committing tests that fail purposely that will only increase, not to mention people will lose faith in the testing framework. For smaller instances such as a development company I think having failing tests works fine as everyone can be aware of the state of the tests.

The issue queue should alert everyone to what bugs exists, not the tests. Since the issue queue has lots of rotting issues it isn't that useful. Cleaning up the issue queue would be a good step, but requires a lot of work and quicker review/committing. The automated testing site that would run all the tests on patches to ensure that they apply and don't break tests would help to that end enormously. That is feasible if we have 100% tests passing. That also works for patches which fix bugs or make API changes as the patch can change the tests as well.

Calling us lazy and such really doesn't make sense since we work on areas that interest us. I work on testing (other stuff as well) not filter stuff. If I choose to go work on that or something else that is my choice. Since I'm not getting paid and don't therefore have a responsibility to do so it doesn't make sense to call me/others lazy.

I agree with the reviewing. People should either review whole thing make the comments and say it is either ready to go after that or still needs work. I personally make sure I follow up on issues that I review.

If your patch works then I think we should get Dries to commit it and leave the bogus cleanups to whoever feels like they want to mess with that.

Well this lazy sod just cleaned that test back up at 2am on Saturday morning. Mea culpa on not checking it properly the first time.

@#16

1 person claims to fix a bug but is actually introducing it. (Confusion on simple arithmetics here.)

Excuse me? There actually was a bug in your patch. "I originally removed the +3 since I don't understand the logic of it. If anything, it should be -3." shows that you do not understand the purpose of the +3 at all, although it is well documented in the line above. Complaining that I made an arithmetic error is simply an unfounded complaint/exaggeration, whereas you just removed code because you didn't understand how it worked. :P

Look at it this way:

http://example.com/ is 19 characters.

Say I set the maximum url length to 18 characters.

This will "trim" it (with your logic) to:

http://example.com... which is actually longer than the original url would have been.

To avoid this, we check for the url being longer than the maximum length + 3:

http://example.com/node will then change to http://example.com... which is indeed shorter, whereas http://example.com/ will remain (rightfully) untouched, since "trimming" it has a negative effect.

Oh my, I didn't even sleep that long and so many comments already... I'll try to reply to some more points:

@boombatower: I certainly didn't call you lazy because you didn't bother to look at my patch. I call those lazy that:
* glance at the patch 10 seconds, point out 1-3 things and call it review. *Especially* if they acknowledge themselves that they didn't look at it all or that there might be more.
* post a new patch that they didn't even superficially test.

If I understood you correctly, we are fully in agreement on this point.

Admitting that my experience of the Drupal core community is very limited, but with the narrow vision I have, it almost looks like a vast majority of key Drupal developers fall into this category. Which brings me to my second point:

@webchick

You might want to consider a change in tactics.

Yes, obviously if my motivation still was to get the patch committed, I wouldn't say this out loud. But since I have nothing to lose... Even then I'm not doing this to vent my personal frustration (putting this in perspective, this is a minor issue in my whole life) I just thought that the thread and events we discuss might be helpful to Dries or someone else, if I summarize it in a concise fashion. I just thought that maybe this will lead to some people recognizing a change in culture or policy would do Drupal good. It may not, and I may be wrong, I just want to say that this is not a personal vendetta of mine, I do have a point I'm trying to make.

But remember that people who are finding whitespace issues today might very well be finding deep logic flaws in Taxonomy module in 6 months, as long as they're given encouragement early on. Your attitude that these contributions are useless does far more harm than good.

I completely disagree on this attitude. My point - which I try to make - is that all of these people were actively harmful to a working patch. They helped a bug stay in Drupal through another year and through 2 releases (assuming it won't make 7.0 either).

In Linux they had this same discussion some time ago. Al Viro summarized the conclusion well in his suggestion to send discussion on whitespace patches to a newly created "linux-wanking" mailing list. (http://lwn.net/Articles/283854/)

The point is that whitespace patches aren't for free either. A person who is not capable of dealing with the code, only the whitespace, is actually a high risk candidate for accidentally breaking something. (As we have seen...) Drupal may not be as demanding as an operating system written in C, but it is the leading most high profile PHP project out there. If you need to practice PHP (or "discipline", which is more likely the problem in this case, I'm sure you all know PHP if you just focus, better than me even) then there are other places to do that than Drupal core! (In Linux they also concluded that people using time and energy on whitespace will never learn to do anything useful.)

There is one aspect to this, the fact that none of *you* know *me*. I'm nobody in Drupal, indeed at the start you had to advice on what arguments you use for diff! So I fully expected things to be slow and getting more scrutiny than average. You *should* treat me with suspicion. Even so, it was surprising to see that there never was any progress, the thread just became a playground for messing up things. That such a thing is allowed to go on in Drupal core warrants some discussion, imho.

***

Then to the main point, Drupal's test driven development: I'm actually *not* against the idea of maintaining a 100% pass rate. It is an approach with many upsides. But not all arguments I've read in favor of it make any sense:

* People ask on IRC whether some tests are supposed to fail or not.

Just ignore them, let them figure it out themselves. (see next) If they can't, you very well *should* ignore them.

* If there are tests failing, there is no way to know if it is because of my patch or not, so I cannot depend on the tests.

Sure there is, sure you can. You run the tests first and make a note of which and how many tests fail. You apply your patch and test again. If more tests failed, you screwed up, if less, you did something good.

Even t.d.o could use the same logic if you wanted to go that route. (Which I'm still not saying would be right for Drupal, I'm just saying it would be completely possible.)

* When I apply my patch and do cvs update, there are tests failing so now I don't know if it is because of me or someone else.

Silly you, should've tested before doing cvs update, then you would've known. (With my experience in the URL filter case, it seems people just take it as normal to never test any patches they create. This is the problem, and having a policy of 100% test rates will never solve it!) Even so, if after an update there are new tests that fail, you obviously need to figure out if HEAD just changed in ways that make your patch bad, or if it is unrelated. (Again, easy to find out, just remove your patch and compare results.)

***

Then take the converse side of things: You maintain 100% pass rate and roll new/changed tests in the same patch with the code. Surely everything is great now? Then you essentially have 4 types of patches in the queue: 1) bug reported, no patch, no tests, 2) bug reported with patch with test case, no code, 3) bug reported with tests and code, but incomplete, 4) bug reported with tests and code, complete

t.d.o now reports:
1) nothing? (no patch)
2) patch fails some tests (which is the whole point at this stage)
3) patch may pass or fail. if it passes you cannot distuingish this from (4) if it doesn't, you can't distinguish from (2)
4) patch passes

So now when a patch fails, people have no idea knowing whether it's because it is (2) or (3). They will assume it is (3), a patch someone is working on but is incomplete, and ignore the issue, when in fact it is a bug waiting to be fixed. When a patch passes, you will not know whether it is a (3) or (4), but due to a false sense of security coming from t.d.o people will assume it is (4).

So I would argue that separating tests and code would actually help seeing what the real state is.

But maintaining 100% pass rate is good for catching regressions, which is important. For instance URL filter isn't a regression, it has always been buggy like it is now. But I just can't wait for the comments when 7.0 is released, with all the bloggers proudly proclaiming that Drupal now has 0 bugs since all tests pass :-)

@cwgordon: I'll take this fight separately from the main issue...

So in your example, http://example.com/ will remain untouched although it is longer than the (user set) allowed length of an URL. How is this correct?

Now that you explained what you are trying to do, I understand your logic. It is not correct, but there is a logic and it is more or less as correct as my code was, which I never claimed to be fully correct either. (So, I'm a lazy bastard too. I just don't see this as a bug that will never harm anyone, so I let it slide. But it took about 5 seconds to get right, so I should be ashamed for my laziness.)

Just to end the discussion, the correct code would have been:

// Cut all strings longer than given length, no more, no less.
if (strlen($text) > $_length && $_length > 3) {
  // Cut 3 extra characters to make room for the 3 dots.
  $text = substr($text, 0, $_length-3) . '...';
}

Since you took the effort to create a new patch solely for this issue, I was just surprised you didn't really correct it. As you note, I didn't oppose your patch, whether there is one dot more or less is such a minor issue. But now that you took it up I had to write the correct code just to end the discussion :-)

But you're right, saying that you "introduced" a bug was an exaggeration (and I used it for dramatic effect, no less!), although you didn't fix it either.

All-in-all where does this leave us? Does the patch still apply and fix the bug? If so point out which one or repost proper patch (maybe the original/older one). Lets get it committed and open up the issue or a new one for cleaning it up further if people so desire.

Then we get the tests back and the bug fixed. I do find it rather absurd that a bug with a patch can go uncommitted for a year without any real reasons against it. I find white-space issue in core fairly often, really not the end of the world.

A future request is bad enough, but a bug...