This is a little experiment stemming from http://drupal.org/node/652420#comment-2347502, in which sun posted a patch that:

  1. Accidentally introduced fatal errors in multiple .test files
  2. Passed the test bot (?!)

I'm trying to see if this is reproducible by calling an undefined function, foo(), in a .test file.

CommentFileSizeAuthor
#21 simpletest-completion-check.patch1.9 KBcarlos8f
#17 kobayashi-maru-drupalget-nocollect.patch2.46 KBcarlos8f
#11 kobayashi-maru-drupalget.patch2.47 KBcarlos8f
#8 kobayashi-maru-chx.patch3.52 KBcarlos8f
#7 kobayashi-maru.patch689 bytescarlos8f
#6 kobayashi-maru.patch713 bytescarlos8f
#2 test-fatal-error-reporting3.patch31.88 KBcarlos8f
#1 test-fatal-error-reporting2.patch28.19 KBcarlos8f
test-fatal-error-reporting.patch540 bytescarlos8f

Comments

carlos8f’s picture

carlos8f commented
StatusFileSize
new test-fatal-error-reporting2.patch28.19 KB

Just as I suspected :-/

Now, merging in the patch from #653940: Clean-up: Tests do not report all errors, to see if that exposes the fatal error.

carlos8f’s picture

carlos8f commented
StatusFileSize
new test-fatal-error-reporting3.patch31.88 KB

OK, that didn't improve the situation. Now merging with #652394: Aggressive watchdog message assertion, per sun's request.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

subscribing

Status: Needs review » Needs work

The last submitted patch failed testing.

carlos8f’s picture

carlos8f commented
Status: Needs work » Closed (fixed)

Fails because of the aggressive watchdog patch, but FilterCRUDTestCase is still disappearing from the results due to the fatal error. Interesting experiment. Further progress will have to be made in the issue, #560646: Fatal PHP errors don't cause tests to fail to fix this.

carlos8f’s picture

carlos8f commented
Title: Test: Does PIFR catch fatal errors in .test files? » Test: Error reporting with SimpleTest/PIFR
Assigned: Unassigned » carlos8f
Priority: Normal » Minor
Status: Closed (fixed) » Needs review
StatusFileSize
new kobayashi-maru.patch713 bytes

Continuing some testing. The following patches and results can be ignored :)

carlos8f’s picture

carlos8f commented
StatusFileSize
new kobayashi-maru.patch689 bytes
carlos8f’s picture

carlos8f commented
StatusFileSize
new kobayashi-maru-chx.patch3.52 KB

kobayashi-maru.patch *should* cause the bot to fail the test, if it catches fatal errors properly. Now merging with #560646: Fatal PHP errors don't cause tests to fail (comment #12).

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

I do not see a difference between #6 and #7, but you somehow managed to make #6 fail.

carlos8f’s picture

carlos8f commented

@sun: #6 had windows line endings. the bot doesn't like them :)

carlos8f’s picture

carlos8f commented
StatusFileSize
new kobayashi-maru-drupalget.patch2.47 KB

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review 
+++ modules/simpletest/tests/error_test.module	11 Dec 2009 00:49:50 -0000
@@ -64,3 +77,14 @@
+function error_test_generate_fatal($collect_errors = FALSE) {
+  // Tell Drupal error reporter to send errors to Simpletest or not.
+  define('SIMPLETEST_COLLECT_ERRORS', $collect_errors);

errr. So all we're missing is this?

This review is powered by Dreditor.

carlos8f’s picture

carlos8f commented
Status: Needs review » Needs work

With #11, I am testing whether fatal errors are caught when the page requested with drupalGet() has a fatal error on it.

Results are:

  • Standard Drupal SimpleTest: caught error
  • PIFR: caught error
  • run-tests.sh: did NOT catch error, passed all tests

Hmm.

carlos8f’s picture

carlos8f commented

@sun: I'm not convinced that SIMPLETEST_COLLECT_ERRORS does anything for catching fatal errors. I think the 'collection' mechanism is simply to record errors in an array for later reference by the test. I could be wrong though! Lots more testing to do on these testing systems.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Priority: Minor » Critical

But your patch in #11 failed?

See http://api.drupal.org/api/function/_drupal_log_error/7

Note that this approach currently only tests fatal errors on the "client site". Not in the parent site, e.g. in tests. For example, see #656300: Typo in system.test prevents D7 from passing tests.

carlos8f’s picture

carlos8f commented
Priority: Critical » Minor
Status: Needs work » Needs review
StatusFileSize
new kobayashi-maru-drupalget-nocollect.patch2.46 KB

Here SIMPLETEST_COLLECT_ERRORS will be FALSE. If this passes, you are right.

I'm not marking this issue critical, since it's simply my private test area for PIFR right now. I'll report back to #560646: Fatal PHP errors don't cause tests to fail when I figure out what the solution is.

I tried to make it clear in #14 that #11 only deals with fatals in drupalGet() responses. Ideally code in .test files and pages invoked by drupalGet() will report fatals securely. I'm aware that if a fatal occurs in a test class, the entire class will disappear from the test results, because each class gets tested within its own HTTP request edit: PHP process or HTTP request. So the way to catch fatals in test classes would be different -- it would involve comparing the number of test classes in the results with the number from before the tests, if they don't match, a fatal error probably occurred and mark the whole test as failed. I don't know anything about how PIFR invokes simpletest yet, so I'll have to research how to implement that tomorrow. So far, I'm getting different pass/fail results in standard Drupal SimpleTest, run-tests.sh, and PIFR for the same code. Kind of frustrating :-/

Status: Needs review » Needs work

The last submitted patch failed testing.

carlos8f’s picture

carlos8f commented

OK, I am installing pifr on a local machine to see what I can do.

glacialheart’s picture

glacialheart commented

subscribing

carlos8f’s picture

carlos8f commented
Status: Needs work » Needs review
StatusFileSize
new simpletest-completion-check.patch1.9 KB

gogogo

carlos8f’s picture

carlos8f commented
Status: Needs review » Closed (fixed)

WOOOO!!!

The results from the test above reveal that the Comment tests are broken in HEAD due to the fatal error I described in http://drupal.org/node/653940#comment-2366180.

I'm taking this patch over to #560646: Fatal PHP errors don't cause tests to fail for review. That was well worth an all-nighter! :-D