Script test runner does not clean after fatal error

Added the test-specific error.log to the review log output.

Apparently, I do not see any output from the new cleanup function in the review log for patch #0 in the testbot review log. However, this works correctly on my local machine. That possibly means that the simpletest_test_id table does not contain a db prefix on the testbot. (?)

Review log for #2 contains the expected cleanup operations after the fatal error.

However, there's a second fatal error in another test case now.

The $test_id appears to be the same for all test runners? How is that possible? For Simpletest module, it changes for every test case being executed in the batch.

I fail to see how {simpletest_test_id}.last_prefix is able to work when tests are executed concurrently.

run-tests.sh calls into simpletest_last_test_get() (like Simpletest module itself), but that is totally not designed for concurrency in parallel threads.

The concurrency problem with last_prefix cannot be fixed without a database schema change, which inherently means it cannot be fixed without adjusting the entire PIFR stack.

The PIFR stack must work across major versions of Drupal, so it would need a conditional, version-agnostic adaption all over the place.

I've no interest in pursuing that.

So while this patch here fixes a tough problem, which causes @jthorson to have to manually reset and clean individual testbots every second day, it's not possible to commit the fix.

Alternatively, let's see what happens if we assign a new test_id for every class. This might blow up though.

Looks like that stop-gap fix worked. Not sure about PIFR implications, but attached patch completes the fix and properly removes the bogus assumption that there's only one $test_id.

oink. Now the review log contains the additional cleanup debug messages for each test case:

Node access and fields 50 passes, 0 fails, and 0 exceptions

- Found database prefix 'simpletest644034' for test ID 280.
Node access rebuild 19 passes, 0 fails, and 0 exceptions

- Found database prefix 'simpletest983969' for test ID 282.
Node access pagination 30 passes, 0 fails, and 0 exceptions

- Found database prefix 'simpletest60768' for test ID 281.
Node access records 26 passes, 0 fails, and 0 exceptions

- Found database prefix 'simpletest433378' for test ID 283.
Node Access on any table 159 passes, 0 fails, and 0 exceptions

That should only be output in case of a fatal error. Need to think how to fix this.

Attached patch simplifies the changes and should resolve the verbose debug output issue for positive tests.

So #13 actually works as intended.

However, as of now, DrupalUnitTestCase only sets and changes the database prefix, but does not update it correctly in Simpletest's test_id table. The patch in #1563620: All unit tests blow up with a fatal error fixes that.

The actual error is supposed to be captured and inserted into SimpleTest's assertion table. In case of fatal errors the actual error is read from the PHP error.log, and this happens after the fact (and is only possible in run-tests.sh, since simpletest.module runs in the same process as the test).

Note that this issue/change is about web/integration tests, not unit tests.

In order to be able to clean up the test environment after a test run with fatal error(s), the test runner needs to know the database prefix that was used to set up the test.

Simpletest normally records the database prefix for each test being run. Simpletest itself attempts to clean up the test environment after each test run. In case of a fatal error, those Simpletest functions are no longer executed.

The current run-tests.sh script does not contain any clean up code currently. This patch basically adds the same clean-up operations to run-tests.sh, which are executed whenever a test runner completes.

However, run-tests.sh supports a special notion of concurrently executing multiple test runner threads in sub-processes, which was only added to the run-tests.sh script, but not to Simpletest. Right now, run-tests.sh uses the same test ID for all tests being executed. This means that, in concurrency, run-tests.sh is not able to retrieve the database prefix that was used to setup the test site, because the concurrently executed test runners overwrite the recorded database prefix. Therefore, this patch changes run-tests.sh to use a separate unique test ID for each test runner, so each test runner can retrieve the database prefix, and in turn, clean up the environment after test execution.

This also implies a functional change -- the configuration option that controls whether the test environment should be cleaned after a test run is ignored.

Unit tests do not properly set up a database prefix currently - even though they are supposed to record any errors and fatal errors in the same way like web tests. That's why this patch depends on #1563620: All unit tests blow up with a fatal error

Note: We're testing these patches in #1591812: Sun's Simpletest patches currently.

Technically, this should work correctly now.

Excellent.

And now. Let's try to kill a bot.

No improvement. :( Sorry.

http://qa.drupal.org/pifr/test/135299 is stuck with another, subsequent test now.

The review log still contains MySQL data file I/O errors. I don't know why.

Technically, the concurrency fix for run-tests.sh worked. But at some point, MySQL suddenly starts with I/O errors on data files.

I wonder whether the testbot client itself attempts to perform additional clean-up operations on its own, which might conflict?

EDIT: The only actual improvement is that the patch/test didn't come back as "green" (with 0 passes).

Right. However, that's exactly what this patch tries to prevent.

To make sure we're not introducing random test failures, I'm resending this patch for re-test all over again a bit... (already done so two times)

#34: drupal8.run-tests-fatal-clean.34.without-fatal.patch queued for re-testing.

heh. Just wanted to cry and ask why and how on earth #44 was able to pass, but "fortunately", only the reported test result is misleading - the review log contains fatal errors.

That is, because this patch depends on the changes in #1563620: All unit tests blow up with a fatal error, so postponing on that :)

The primary dependency landed: #1541958: Split setUp() into specific sub-methods

The second dependency is: #1563620: All unit tests blow up with a fatal error

After that, we can backport this one (whereas this patch is the most straightforward of the chain).

The primary purpose of backporting these changes was to retain a comparable testing framework between D8 and D7, so as to make future backports easier.

Getting close to D8 feature freeze, and facing a heavily diverged simpletest framework between D8 and D7, it's close to impossible to retain "similarity" between major core versions, and doing so involves a huge risk of breaking things badly in D7.

Therefore, I'm calling out the end to testing framework backports.

For D7, you get to work with the current mess. At the very least, that's "predictable."

Needless to say, actual bug fixes are excluded from that. However, given that there are 30,000+ test assertions that pass against D7, people will have to make pretty good arguments and show solid proof that there is actually a bug somewhere. ;)

Let's focus on the future. (Which isn't Simpletest either way.)

Thanks everyone for working on this and making Drupal awesome! :)