Fatal error during batch operations causes endless test loop

Can confirm this issue, already asked for some feedback on IRC but no luck there (someone tried reproducing it by adding some random characters to a file, that's actually not enough, calling a method on a non object is a reliable way to loop the test [or $this->test()] where test() is not a valid function)

Some information:
simpletest.module will run until line 325 $test->run(); and never finishes it's testing loop. The reason seems to be that the batch information is somehow lost between runs.

Here is a screenshot of my database:

Fixing the fatal error completes the test, btw.

Can someone upload a do not test patch so people can replicate the issue without having to think too much.

To create a do not test patch add "do-not-test" before the .patch. For example "drupal.do_nothing_555555-do-not-test.patch"

Here is a patch. It adds a line to the Form\ElementTest.
And a screenshot of it looping:

Just reproduced this with my patch at home. Forgot it was running...

09:01:31 {8.0.x} /var/www/drupal$ drush test-clean
Removed 836 leftover tables.                                        [status]
Removed 36 temporary directories.                              [status]
Removed 1 test result.                                                  [status]

But I just managed to have a PHP fatal error without the loop:

Can confirm, works with beta11, broken in 7d532f7..., only difference seems to be the version constant.

Managed to cause a loop while not testing:

The batch page isn't happy about php fatal errors it seems.
Example: Search_API currently has broken indexing and uses the batch page for indexing. When adding a fatal error to Index::indexItems and clicking 'Index now' on /admin/config/search/search-api/index/default_index, the batch will never finish.

I think this is related to either display_errors or xdebug being enabled.

Also strange.. When I just start a batch, it loops forever. But when I then manually refresh the page while it's looping, its' running once more and then displays the xdebug error message below the "An error happened" page.

Just adding a tag

We'll want to test this.

Here is my try for a test for this error. It loops in the browser (unless you hit refresh), I think the testbot won't loop, but it won't reach the second assertion and have the standard test fail The test did not complete due to a fatal error..

Setting to needs review to get a testbot run for this patch.

Confirmed the patch in #18 loops through the UI, and finishes from CLI (it doesn't pass on CLI though--we'll probably need a catchable fatal error, rather than an outright fatal which isn't catchable).

I also confirmed that the looping doesn't occur if display_errors is off (the batch response at that point is a 500). When turned on, the response is a 200! Enabling/disabling xdebug doesn't appear to impact.

So, something in the batch system is changing the response code to a 200 if display_errors is turned on.

500 vs 200 is the standard PHP behavior for display_errors being on or off. No idea why it does that, but it AFAIK always behaved like this. I don't understand what changed in batch API, but somehow, we need to treat any response that can't be parsed as an error and abort, instead of trying to do something with it.

500 vs 200 is the standard PHP behavior for display_errors being on or off

Good to know!

I've tested this without JS, and it fails as expected upon encountering a fatal error. So, this is only an issue with the JS-processing of responses. I'll dig into this now.

It appears that the removal of dataType in the ajax request from #2481453: Implement query parameter based content negotiation as alternative to extensions. Is responsible. Without that parameter, the ajax method won't detect invalid json as a problem, and happily continue on even though progress.status and progress.data aren't set.

Removing the 'needs tests' tag, because I don't think this will be reproducible without javascript tests.

This patch fixes the issue in my manual testing with the patch from #18 applied.

Since this is a bug in progress.js, this has wider implications than just testing. It could mask fatal errors during any other batch operation using JS.

Retitling per #27.

@alexpott and I agreed on this being an RC target.

Seems like this is batch system and not simpletest.

What do we need to do to test this one-line patch and get it in?

I just tried this again, and the issue still occurs, and the patch in #25 fixes it. To reproduce, and test the fix:

1. Download and apply only the patch from #18
2. Browse to /admin/config/development/testing
3. Open the chrome network inspector (or equivalent)
4. Run SimpleTestTest
5. Note the batch requests just keep on going:
6. Download and apply the patch from #25. Make sure to clear caches so updated js is used
7. Run SimpleTestTest again. The error is caught because the returned fatal error is invalid json:

Confirmed that the test steps in #34 fix the problem.

I also confirmed this separately by introducing a deliberate fatal error in the Ubercart test suite. I had seen this issue before but was unsure of the cause - fatal errors cause the Simpletest UI to run in an infinite loop without the patch in #25, but with the patch in #25 tests with errors stop as expected with an error message after a single run.

Committed/pushed to 8.0.x, thanks!