Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
This complements #652394: Aggressive watchdog message assertion.
But it is not the same, since e.g., an unexpected Access denied page or an unexpected severe watchdog log message is no programming error.
I hope that this patch fails, because there are a couple of fails in the other patch that I don't fully grok.
Comment | File | Size | Author |
---|---|---|---|
#59 | 653940-revert-cruft.patch | 8.1 KB | Damien Tournoud |
#55 | fix-fatals-revert-reporting.patch | 6.1 KB | carlos8f |
#53 | drupal.revert-reporting.53.patch | 5.77 KB | sun |
#51 | drupal.revert-reporting.50.patch | 4.98 KB | sun |
#48 | quick-fix-to-comment-tests.3.patch | 2.47 KB | carlos8f |
Comments
Comment #1
sunComment #2
sunComment #4
sunComment #6
sunWe produce invalid XHTML.
http://bugs.php.net/bug.php?id=46136
http://de2.php.net/manual/en/function.libxml-use-internal-errors.php
Until this the markup is fixed, let's suppress validation errors during HTML parsing.
Comment #8
sunComment #10
sunComment #11
sunComment #12
sunReverted that static setUp() in field.test. Not sure how to fix that properly. :-/
Comment #14
sunComment #15
sunComment #17
carlos8f CreditAttribution: carlos8f commentedsubscribing.
Comment #18
sunok, found the solution for the OOP problem myself.
This one should get us (far) below the 100 exceptions treshold. Since we are on PHP5 now, we can (and actually need to) use some new filter function awesomeness to prevent run-time notices.
Comment #19
carlos8f CreditAttribution: carlos8f commentedI don't understand why the pifr test results page doesn't allow you to inspect some of the tests that had exceptions. Only certain ones are clickable w/ details. How is it that you know what to fix?
Oh, and I'm glad someone is addressing these exceptions. Thank you! If we can get the exception count down to zero and commit this, it would make future testing much more robust.
Comment #20
sunOnly the first 20-50 errors are displayed in the results. Changing this is already requested and scheduled.
This one will fix the fails and decrease the exceptions to ~5.
I have no idea how to resolve the failing rmdir() and other file tests though. I badly need help for those.
Comment #22
sunZERO.
Comment #23
carlos8f CreditAttribution: carlos8f commentedooh,,,, I can't wait for it to turn green :P
Comment #24
boombatower CreditAttribution: boombatower commented#560646: Fatal PHP errors don't cause tests to fail....
Comment #25
carlos8f CreditAttribution: carlos8f commentedOops, sorry for the cross-post in #19.
Whoo hoo! Test bot passed it, this should definitely be committed now.
sun, I tip my virtual hat in your general direction. Thank you!
Comment #26
sun@boombatower: Not sure what you tried to say. This patch is about far more than fatal errors.
But speaking of fatal errors:
Comment #27
sunIn addition to have pinged Dries + webchick about this patch, I'm changing the title in the hope that this patch really goes in first.
I naturally expect a couple of patches in the RTBC queue to fail afterwards. And that will be the best that can happen.
Comment #28
suncarlos8f took this patch + merged a fatal error into it in #655814: Test: Error reporting with SimpleTest/PIFR
The result is that the testbot still does not catch fatal errors. But well, a single patch cannot save the world, I guess...
Comment #29
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD.
Comment #30
Damien Tournoud CreditAttribution: Damien Tournoud commentedErm. What? Why? Please document.
Erm. What? Why? We already have that in drupal_environment_initialize():
The
ini_set('log_errors')
should not do anything, because all errors on the testing side should be caught by the error handler.Erm. What? Why? If error_reporting() is already E_ALL, what's the point?
Comment #32
sunWhat exactly is unclear? See also #6
If the rest is true, then both patches should fail.
Comment #33
Damien Tournoud CreditAttribution: Damien Tournoud commentedOk, let's revert that and think about it a little bit more.
For example this is *very* wrong:
... because you are showing errors that were hidden (like by using the @ operator).
Comment #34
sunIf you want so...
Comment #35
Damien Tournoud CreditAttribution: Damien Tournoud commentedLooking at #22 closely, there was absolutely zero critical issue in there. All those are either (1) stuff that were hidden by using the @ operator, which you reveled by braking the error handler, or (2) mundane errors like not passing a variable as a per-reference parameter like this:
I guess this is actually a E_STRICT error? In that case, the best way would have been better dealt with inside #348448: Always report E_STRICT errors.
Comment #36
sun?
AFAICS, one difference to #348448: Always report E_STRICT errors is that errors are limited to the testbot and not exposed during normal operation. Aside from that, the patch over there mostly contains the same fixes.
Comment #37
carlos8f CreditAttribution: carlos8f commentedThis introduces a fatal error, because postComment() does $this->drupalGet(). This is illegal for a static function to call!
In my SimpleTest environment, running Comment tests crashes with:
When running run-tests.sh, I also received:
Instead of "Comment stuff X passes, X fails, and X exceptions", etc. Removing this change causes tests to pass again.
The question is, WHY DOES PIFR NOT REPORT THIS??? Our patches are not checked properly by qa.drupal.org. See #560646: Fatal PHP errors don't cause tests to fail for discussion and progress.
We need to take some time to examine our testing platform, since obviously it has not done its job here.
I'm on crack. Are you, too?
Comment #38
boombatower CreditAttribution: boombatower commentedI believe I have documented the reasons as well as I know how at #560646: Fatal PHP errors don't cause tests to fail. This is not a PIFR issue, until someone explains how that is possible. The integration of PIFR and simpletest is extremely minimal it grabs results directly from simpletest table.
The reason this happens on bot is that they run with very high concurrency settings > 4 most of the time (even 16). The higher the concurrency setting the better chance this occurs. I stress the word chance as it will not always occur. I am not sure if I need to explain the current error catching code in greater detail on the other issue or what as it seems to be misunderstood.
Comment #39
carlos8f CreditAttribution: carlos8f commentedI agree that SimpleTest (at least in the context of run-tests.sh) needs to report these errors correctly for PIFR to parse, but in addition, the problem also has to do with PIFR not doing a proper sanity check on the results, namely that no classes in the test batch have return zero results because they were aborted somehow.
Comment #40
carlos8f CreditAttribution: carlos8f commentedExcited that DamZ found a simple fix to run-tests.sh, which may fix the SimpleTest side of things.
I still think the PIFR sanity check would be a good idea. From what I understand PIFR would need to run run-tests.sh --list to enumerate, then double-check that all the results came back at the end.
Comment #41
boombatower CreditAttribution: boombatower commentedDefinitely a possibility, but also then that depends on --list working. At some point PIFR must depend on simpletest.
As for simple fix, it is simple in theory...I have yet come up with a clean easy way with current code base to get db_prefix, which part of my reason for overhaul.
Comment #42
carlos8f CreditAttribution: carlos8f commentedIf --list comes back with nothing, obviously something is wrong, mark the patch as failed. so that's sanity check part 1.
I would love to see your patch, even if it doesn't have a the db_prefix yet. Is there an issue?
Comment #43
boombatower CreditAttribution: boombatower commentedPosted partial patch: #560646: Fatal PHP errors don't cause tests to fail.
Comment #44
carlos8f CreditAttribution: carlos8f commentedThe solution to expose fatal errors has been found in #560646: Fatal PHP errors don't cause tests to fail, and I am rolling a simple patch to fix the fatals introduced here. This must be committed before we can fix SimpleTest.
The current issue still needs to be examined for consistency, as per DamZ's concerns. I give props to sun, but it might've been committed a little too hastily.
Comment #45
carlos8f CreditAttribution: carlos8f commentedcrap. windows newline bytes for the loss :P
Comment #47
carlos8f CreditAttribution: carlos8f commentedSheesh. UserCancelTestCase->testUserDelete() is broken since it uses CommentHelperCase::postComment(), which cannot be called statically since it uses $this. After fixing this and running User tests locally, I've found even more errors. Looking into this.
Comment #48
carlos8f CreditAttribution: carlos8f commentedThe fix wasn't as 'quick' as I thought. I had to make UserCancelTestCase extend CommentHelperCase, which probably isn't the ideal architecture so I added some TODO comments.
This patch should at least fix fatals in HEAD.
Comment #49
carlos8f CreditAttribution: carlos8f commentedThis patch is good to go, being a quick-fix.
Comment #50
webchickUm, no. Sorry. I can't commit this patch. We totally cannot have a required module's tests (user) extending from an optional module's tests (comment). Furthermore, that just simply doesn't make any sense. ;P
Can you explain a bit more what the problem is? Do we need to move stuff from CommentHelperTest into DrupalWebTestCase, or..?
Comment #51
sunMerged that and cleaned it up.
Comment #52
carlos8f CreditAttribution: carlos8f commentedwebchick: i apologize. Agreed that the inheritance was a WTF. sun is re-working the patch to use drupalPost() instead of postComment(), which is quite brilliant :)
#51 came before the IRC conversation about this. That patch can be ignored.
Comment #53
sunBlind patch.
Comment #55
carlos8f CreditAttribution: carlos8f commentedsun's patch with a trivial fix.
Comment #56
sunComment #57
webchickI can't read this well enough to determine if this addresses DamZ's concerns, but it does mine. I at first thought we should move postComment to DWTC, but I see that it encapsulates a heck of a lot of edge cases which make total sense for comment testing, but not so much for run-of-the-mill comment posting in tests.
Committed to HEAD. So excited to see this long-standing issue finally coming to a close! :D
Comment #58
spelzmann CreditAttribution: spelzmann commentedIt's nice to see those repeated red bars turn to green :]
Comment #59
Damien Tournoud CreditAttribution: Damien Tournoud commentedLet's also revert the rest of the cruft introduced by the original patch.
Comment #60
carlos8f CreditAttribution: carlos8f commentedPhew, lots of issues with d.o today, CVS server was down for a while, so I couldn't grab HEAD or get to d.o to review this. I tried posting this comment multiple times and get a WSOD. Grr!
Looks good, I agree that all these errors are totally suppress-able, and shouldn't have shown up since
if ($severity & error_reporting()) {
was mistakenly removed.Thanks ;)
Comment #61
webchickCommitted to HEAD. Thanks!
Comment #64
PC Pro Schools CreditAttribution: PC Pro Schools commentedHah, you can only imagine the relief the coders and hackers feel when they finally get the green light ;D