Right now if an exception is thrown during a test's setUp() or tearDown() method, the testbot shows a "CI error". See discussion at #2189345-192: run-tests.sh should exit with a failure code if any tests failed for additional details.

Overall it would be better if that exception were handled by Drupal itself.

Drupal 8 looks like it fixed this in #2170023: Use exceptions when something goes wrong in test setup.

CommentFileSizeAuthor
#2 2763435-2.patch2.19 KBDavid_Rothstein
#2 2763435-2-with-teardown-exception.patch2.62 KBDavid_Rothstein
#2 2763435-2-teardown-exception-without-the-fix.patch445 bytesDavid_Rothstein
#2 2763435-2-with-setup-exception.patch2.61 KBDavid_Rothstein
#2 2763435-2-setup-exception-without-the-fix.patch429 bytesDavid_Rothstein

Issue fork drupal-2763435

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

David_Rothstein created an issue. See original summary.

David_Rothstein’s picture

David_Rothstein commented
Status: Active » Needs review
StatusFileSize
new 2763435-2-setup-exception-without-the-fix.patch429 bytes
new 2763435-2-with-setup-exception.patch2.61 KB
new 2763435-2-teardown-exception-without-the-fix.patch445 bytes
new 2763435-2-with-teardown-exception.patch2.62 KB
new 2763435-2.patch2.19 KB

Here is a patch to try to fix it. This essentially is a pseudo-backport of a small part of #2170023: Use exceptions when something goes wrong in test setup. It is possible more should be backported too (at least the tests that were added there).

This also contains patches for testing purposes that throw exceptions in setUp()/tearDown() so we can see if the fix here is doing what it is supposed to.

The last submitted patch, 2: 2763435-2-setup-exception-without-the-fix.patch, failed testing.

The last submitted patch, 2: 2763435-2-with-setup-exception.patch, failed testing.

The last submitted patch, 2: 2763435-2-teardown-exception-without-the-fix.patch, failed testing.

The last submitted patch, 2: 2763435-2-with-teardown-exception.patch, failed testing.

David_Rothstein’s picture

David_Rothstein commented

So based on the above this patch is basically doing what it's expected to.

Although the results compared to the Drupal 8 patch from https://www.drupal.org/node/1970534#comment-11383991 show some interesting differences that I don't immediately understand. (Maybe it has something to do with the fact that I added the exception to a web test for Drupal 7 but to a unit test for Drupal 8?)

Above Drupal 7 patch with setUp exception:

https://www.drupal.org/files/issues/2763435-2-with-setup-exception.patch

Results:
https://www.drupal.org/pift-ci-job/364173

exception: [Uncaught exception] Line 1250 of modules/system/system.test:
Exception: This is a test exception during setup. in DateTimeFunctionalTest->setUp() (line 1250 of /var/www/html/modules/system/system.test).

Results on dispatcher:
https://dispatcher.drupalci.org/job/default/169140/consoleFull

14:44:43 Date and time 0 passes, 0 fails, and 1 exception

Drupal 8 patch with setUp exception:

https://www.drupal.org/files/issues/2189345-setup-exception-D8_0.patch

Results:
https://www.drupal.org/pift-ci-job/363917

fail: [Other] Line 808 of core/tests/Drupal/Tests/Core/UrlTest.php:
Drupal\Tests\Core\UrlTest::testFromRouteUriWithMissingRouteName
Exception: This is a test exception during setup.

Results on dispatcher:
https://dispatcher.drupalci.org/job/default/169015/consoleFull

04:48:48 Drupal\Tests\Core\UrlTest                                      0 passes  91 fails                            
04:48:48 FATAL Drupal\Tests\Core\UrlTest: test runner returned a non-zero error code (2).
04:48:48 Drupal\Tests\Core\UrlTest                                      0 passes   1 fails

Especially interesting is that Drupal 8 is still getting the "test runner returned a non-zero error code (2)" error here.

David_Rothstein’s picture

David_Rothstein commented

Oh, it could actually have something to do with the fact that I threw the exception before the parent::setUp() in Drupal 7 but after in Drupal 8.

May be worth trying a comparison again with a more exactly equivalent test...

Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
commented

The unit vs web test is more likely the culprit here. With unit tests we hand off execution to phpunit, which, if there is a fatal error, we dont really have any way of recovering from that, beyond "non zero error code", whereas if it fatals inside of drupal code, we have the opportunity to catch that exception and report on the why of the failure.

fjgarlin made their first commit to this issue’s fork.

fjgarlin opened merge request !5189

fjgarlin’s picture

fjgarlin commented

A very similar fix was made as part of #3397117: SQLite testing in GitlabCI - apache2(98)Address already in use but it was taken out in favor of this existing issue.
Exceptions thrown in setUp and tearDown are now caught and therefore reported properly to the CI system.

bramdriesen’s picture

bramdriesen
he/him
Primary language Dutch
Location Belgium 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

Came here from #3397117: SQLite testing in GitlabCI - apache2(98)Address already in use after a review. I think we can RTBC this one here as well! I checked both the PIFT and the GitLab CI results and all seem fine.

Also wanted to say that #2 showed the issue in a very understandable way :)

poker10’s picture

poker10 commented
Issue tags: +Pending Drupal 7 commit

I have reviewed the MR and it looks good to me. Compared with the #2, the $this->tearDown(); is also moved to the try-catch block, but I think this solution is better, because we probably want also tearDown() to run without any exceptions. If there are any problems, it would be better to stop the execution with the exception, instead of "seeing what will happen" next.

Tests are also green. Adding a tag. Thanks all!

  • mcdruid committed d98b9ec5 on 7.x 
    Issue #2763435 by David_Rothstein, fjgarlin, BramDriesen, Mixologic,...
mcdruid’s picture

mcdruid commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -Pending Drupal 7 commit

Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.