Follow up to #2863262-53: Bootstrap: Convert system functional tests to phpunit

UncaughtExceptionTest should be cleaned up and not be made to use cUrl. Some attempts were made to do this in #2863262: Bootstrap: Convert system functional tests to phpunit but DrupalCi has issues displaying errors. It seems, when using Guzzle (or drupalGet) it will always show a white page.

We should find out why this is and maybe use that to clean up the whole call stack when dealing with exceptions in tests.

CommentFileSizeAuthor
#17 2987980-17.patch10.26 KBalexpott
#12 interdiff.2987980.9-12.txt1.28 KBlongwave
#12 2987980-12.patch10.1 KBlongwave
#9 2987980-9.patch9.5 KBalexpott
#9 8-9-interdiff.txt885 bytesalexpott
#8 2987980-8.patch9.43 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jibran created an issue. See original summary.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented
Parent issue: » #2863262: Bootstrap: Convert system functional tests to phpunit
Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude as a volunteer and at Dx Experts commented
Title: Refactor UncaughtExceptionTest » Refactor UncaughtExceptionTest to not use cUrl
Issue summary: View changes
Status: Postponed » Active

@jibran thanks for opening this! Updated the IS to include some of the work done on this.

Something like #48 #2863262-48: Bootstrap: Convert system functional tests to phpunit would be nice I think, if we can get it to work on CI.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Active » Needs review
FileSize
2987980-8.patch9.43 KB

Ah yes this will make debugging this test and to use it for better testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
FileSize
8-9-interdiff.txt885 bytes
2987980-9.patch9.5 KB

Completing the sentence...

The last submitted patch, 8: 2987980-8.patch, failed testing. View results

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude as a volunteer and at iO commented 
+++ b/core/tests/Drupal/FunctionalTests/Bootstrap/UncaughtExceptionTest.php
@@ -56,6 +42,9 @@ protected function setUp(): void {
+    $settings_php .= "\ndefine('SIMPLETEST_COLLECT_ERRORS', FALSE);\n";

Nice! So that was the trick I kept missing :)

While we are cleaning this up, should we remove the 

$this->config('system.logging')
      ->set(xyz)

in \Drupal\FunctionalTests\Bootstrap\UncaughtExceptionTest::testUncaughtException too? That doesn't do anything since we then set this config in settings.php which will always take precedence. So leaving this in creates a sense of testing something we aren't actually testing.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave commented
FileSize
2987980-12.patch10.1 KB
interdiff.2987980.9-12.txt1.28 KB

Agree with #11, let's remove those lines as they do nothing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented

@Lendude @longwave I was thinking to leave these there - they ensure the test runner env and the site under test have the same config. writeSettings - doesn't flush the config cache. But no biggie. I guess for me it's just not part of the scope here.

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude as a volunteer and at iO commented
Status: Needs review » Reviewed & tested by the community

Yeah, not sure we needed to remove that, that's why I asked, but this feels like a clean up and leaving the code in makes it look like it's needed when it's not (in this case...). Leaving it in sorta gives a false sense of security that we are testing that setting when it's in config when we are actually not testing it at all. So all in all ¯\_(ツ)_/¯

But this looks ready to me, very nice!

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

++ now only 2 tests remains infected by curl

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Looks good but needs a re-roll.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Needs work » Reviewed & tested by the community
FileSize
2987980-17.patch10.26 KB

Rerolled...

  • catch committed a857ef3 on 9.2.x 
    Issue #2987980 by alexpott, longwave, Lendude, jibran: Refactor...

  • catch committed 8075cda on 9.1.x 
    Issue #2987980 by alexpott, longwave, Lendude, jibran: Refactor...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 9.2.x and 9.1.x, thanks!

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Issue tags: -Needs reroll
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Version: 9.2.x-dev » 9.1.x-dev

Status: Fixed » Closed (fixed)

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