Clean-up: Tests do not report all errors

subscribing.

I 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.

ooh,,,, I can't wait for it to turn green :P

#560646: Fatal PHP errors don't cause tests to fail....

Oops, 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!

Committed to CVS HEAD.

     if (!$this->elements) {
-      // DOM can load HTML soup. But, HTML soup can throw warnings, suppress
-      // them.
-      @$htmlDom = DOMDocument::loadHTML($this->content);
-      if ($htmlDom) {
+      // Suppress all libxml warnings during loading of HTML.
+      // @todo Remove this when core produces XHTML valid output.
+      libxml_use_internal_errors(TRUE);
+      $document = new DOMDocument();
+      $result = $document->loadHTML($this->content);
+      if ($result) {
         $this->pass(t('Valid HTML found on "@path"', array('@path' => $this->getUrl())), t('Browser'));
         // It's much easier to work with simplexml than DOM, luckily enough
         // we can just simply import our DOM tree.
-        $this->elements = simplexml_import_dom($htmlDom);
+        $this->elements = simplexml_import_dom($document);
       }
     }

Erm. What? Why? Please document.

+    // Log fatal errors.
+    ini_set('log_errors', 1);
+    // Make all errors visible.
+    error_reporting(E_ALL);

Erm. What? Why? We already have that in drupal_environment_initialize():

  // Enforce E_ALL, but allow users to set levels not part of E_ALL.
  error_reporting(E_ALL | error_reporting());

The ini_set('log_errors') should not do anything, because all errors on the testing side should be caught by the error handler.

-    if ($severity & error_reporting()) {

Erm. What? Why? If error_reporting() is already E_ALL, what's the point?

Ok, let's revert that and think about it a little bit more.

For example this is *very* wrong:

-    if ($severity & error_reporting()) {

... because you are showing errors that were hidden (like by using the @ operator).

Looking 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:

-  $format = array_shift(filter_formats($account));
+  $formats = filter_formats($account);
+  $format = reset($formats);

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.

+++ modules/comment/comment.test	9 Dec 2009 23:08:20 -0000
@@ -21,8 +21,11 @@ class CommentHelperCase extends DrupalWe
-  function postComment($node, $comment, $subject = '', $contact = NULL) {
+  public static function postComment($node, $comment, $subject = '', $contact = NULL) {

This 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:

An error occurred.
Path: /batch?id=3&op=do
Message:
Fatal error: Using $this when not in object context in modules/comment/comment.test on line 37

When running run-tests.sh, I also received:

Fatal error: Using $this when not in object context in modules/comment/comment.test on line 37

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?

I 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.

I 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.

Excited 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.

Definitely 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.

If --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?

Posted partial patch: #560646: Fatal PHP errors don't cause tests to fail.

The 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.

crap. windows newline bytes for the loss :P

Sheesh. 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.

The 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.

This patch is good to go, being a quick-fix.

Um, 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..?

webchick: 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.

sun's patch with a trivial fix.

I 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

It's nice to see those repeated red bars turn to green :]

Let's also revert the rest of the cruft introduced by the original patch.

Phew, 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 ;)

Committed to HEAD. Thanks!

Hah, you can only imagine the relief the coders and hackers feel when they finally get the green light ;D