As per the parent issue.

./Bootstrap/ErrorContainer.php
./Bootstrap/ExceptionContainer.php
\Drupal\system\Tests\System\UncaughtExceptionTest

Some test logic got simplified in testMissingDependencyCustomErrorHandler(), see #25

We tried not using cUrl, but failed to got to run this successfully on DrupalCi (worked great locally).

CommentFileSizeAuthor
#51 2863262-26-BTB.patch9.42 KBLendude
#48 2863262-48.patch15.96 KBLendude
#48 interdiff-2863262-46-48.txt7.04 KBLendude
#46 2863262-46.patch14.29 KBLendude
#46 interdiff-2863262-42-46.txt1.25 KBLendude
#42 2863262-42.patch13.6 KBLendude
#42 interdiff-2863262-39-42.txt1.04 KBLendude
#39 2863262-39.patch13.69 KBLendude
#39 interdiff-2863262-36-39.txt4.75 KBLendude
#36 2863262-36.patch11.33 KBLendude
#36 interdiff-2863262-26-36.txt4.79 KBLendude
#32 2863262-26-BTB.patch9.42 KBLendude
#29 convert_to_phpunit.patch2.27 KBhi_ten_ja
#29 interdiff_convert_to_phpunit.txt2.27 KBhi_ten_ja
#26 interdiff-21-26-BTB.txt3.01 KBAnonymous (not verified)
#26 2863262-26-BTB.patch9.42 KBAnonymous (not verified)
#25 interdiff-21-25-BTB.txt9.41 KBAnonymous (not verified)
#25 2863262-25-BTB.patch15.24 KBAnonymous (not verified)
#21 2863262-21-JTB.patch7.99 KBLendude
#21 2863262-21-BTB.patch7.97 KBLendude
#6 2863262-6.patch3.48 KBjofitz
#6 interdiff-2-6.txt3.42 KBjofitz
#2 2863262-2.patch3.58 KBjofitz
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Jo Fitzgerald created an issue. See original summary.

jofitz’s picture

Assigned: jofitz » Unassigned
Issue summary: View changes
Status: Active » Needs review
Parent issue: #2862885: Batch: Convert system functional tests to phpunit » #2862508: Convert system functional tests to phpunit
FileSize
3.58 KB
  • Moved both files.
  • Corrected namespace.
  • Corrected references to the files elsewhere.
dawehner’s picture

@Jo Fitzgerald
Thank you for creating the issues and patches! That is great.

It feels like the right location though would be core/tests/Drupal/FunctionalTests, but I guess this is something we should not address as part of this conversion.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

The changes are as minimal as they should be IMHO. We could leave the custom php classes around, but I think moving them along is the right thing to do.

dawehner’s picture

Status: Reviewed & tested by the community » Needs work

Klausi and I talked about this and we agreed we should actually indeed move to files to the core/tests/Drupal/FunctionalTests/Bootstrap folder and adapt the namespace to Drupal\FunctionalTests\Bootstrap

Sorry I didn't take that into account before properly.

jofitz’s picture

Status: Needs work » Needs review
FileSize
3.42 KB
3.48 KB

Moved the files.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you @Jo Fitzgerald!

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

Shouldn't we also be making \Drupal\system\Tests\System\UncaughtExceptionTest a BrowserTestBase test in this issue too?

dawehner’s picture

@alexpott
This test certainly could be placed there, good idea!
We could, but this additional dimension would make it quite hard to review and work on issues, aka. you would have to look through all possible tests.

alexpott’s picture

@dawehner basically I don't get why we'd move the fixtures before move the test that uses. They seem to belong together - no?

dawehner’s picture

Status: Needs review » Needs work

@dawehner basically I don't get why we'd move the fixtures before move the test that uses. They seem to belong together - no?

Fair, I see what you mean. Yeah let's keep the stuff around for now.

michielnugter’s picture

Issue summary: View changes
Issue tags: +phpunit initiative
dawehner’s picture

@Jo Fitzgerald
Do you need help to solve this issue?

jofitz’s picture

@dawehner What was the conclusion of your discussing with @alexpott? Do we need to move UncaughtExceptionTest?

dawehner’s picture

@Jo Fitzgerald
There are two possibilities:

  1. Convert UncaughtExceptionTest and move the fixtures to core
  2. Don't convert UncaughtExceptionTest, dont' move the fixtures, but then also don't convert the tests which needs these fixtures

What do you think? I'd argue the first one seems a bit more reasonable.

jofitz’s picture

@dawehner I think I'll step back from this issue, I'm getting more and more confused. Sorry.

dawehner’s picture

@Jo Fitzgerald
Which parts do you not understand?

jofitz’s picture

@dawehner There are a few things, including:

  • I'm not entirely sure what you mean by "the fixtures" - can you clarify, please?
  • I tried moving UncaughtExceptionTest, but wasn't sure exactly where it should end up. My first thought is core/tests/Drupal/FunctionalTests/System/UncaughtExceptionTest.php. Did you have something in mind?
  • I also tried converting to BTB, but 7 tests failed and that was the final straw.

Perhaps we could have a chat on IRC at some point?

dawehner’s picture

Perhaps we could have a chat on IRC at some point?

Sure, I think something next week when I'm back in europe would be totally cool. Just ping me, and I'll be able to respond :)

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Lendude’s picture

Status: Needs work » Needs review
FileSize
7.97 KB
7.99 KB

UncaughtExceptionTest is an interesting case (and ++ to whoever wrote the errors, funny stuff).

Converting to BrowserTestBase means the tests stop when an exception is thrown during drupalGet(), so anything after that won't run, or if you try/catch, you can't run any assertions against the content because it was never set.

Converting this to JavascriptTestBase means this isn't a problem, it just looks at the rendered HTML, but then we run into problems using assertResponse which we can't use in a JavascriptTest.

So needs work, but posting progress.

The last submitted patch, 21: 2863262-21-BTB.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 21: 2863262-21-JTB.patch, failed testing. View results

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
15.24 KB
9.41 KB

(and ++ to whoever wrote the errors, funny stuff).

Absolutely! Art from @znerol! Exquisite work!


Converting this to JavascriptTestBase means this isn't a problem, it just looks at the rendered HTML, but then we run into problems using assertResponse which we can't use in a JavascriptTest.

Really. Hardly anyone will tell us thanks when will translate this test into the Selenium :( Although this is a fairly heuristic idea, @Lendude++! Tempting simplicity!


Okay. Then let's leave it at the BTB level via custom cURL request. I'm sure that CurlHelperTrait will be useful to us and in other issues too.
It seems now there is only one place with an ambiguous replacement (at first glance):
+++ b/core/tests/Drupal/FunctionalTests/Bootstrap/UncaughtExceptionTest.php
@@ -142,25 +143,9 @@ public function testMissingDependencyCustomErrorHandler() {
-    $this->drupalGet('broken-service-class');
-    $this->assertResponse(418);
-    $this->assertRaw('Oh oh, flying teapots');
-
-    $message = 'Argument 1 passed to Drupal\error_service_test\LonelyMonkeyClass::__construct() must be an instance of Drupal\Core\Database\Connection, non';
-
-    $this->assertNoRaw('The website encountered an unexpected error.');
-    $this->assertNoRaw($message);
-
-    $found_exception = FALSE;
-    foreach ($this->assertions as &$assertion) {
-      if (strpos($assertion['message'], $message) !== FALSE) {
-        $found_exception = TRUE;
-        $this->deleteAssert($assertion['message_id']);
-        unset($assertion);
-      }
-    }
-
-    $this->assertTrue($found_exception, 'Ensure that the exception of a missing constructor argument was triggered.');
+    $this->curlGet('broken-service-class');
+    $this->assertSame(418, $this->curlGetStatusCode());
+    $this->assertSame('Oh oh, flying teapots', $this->curlGetResponse());

When I saw a partial removal from #21, I thought: "Hey, it seems you are deleting part of the logic, I will return it". But after a more detailed study, I removed even more :)

Why? Because it really not necessary. A detailed explanation:

$this->assertNoRaw($message);
After this code, $message already folds into assertions. Therefore, you can set any value to $message, and WTB-test already will pass.
Example

-    $message = 'Argument 1 passed to Drupal\error_service_test\LonelyMonkeyClass::__construct() must be an instance of Drupal\Core\Database\Connection, non';
+    $message = 'Oh oh, Flying Teapot here!';

Instead of two asserts:

  • the response contains our message,
  • the response not contains default message.

We can do one asserts:

  • the response equals to our message.

$this->assertSame('Oh oh, flying teapots', $this->curlGetResponse())

2. The logic with foreach (assertions) and unset($assertion) also necessary only for pass WTB-test, but not for BTB-test.

Anonymous’s picture

Another approach is to add all the necessary methods inside the UncaughtExceptionTest.

Status: Needs review » Needs work

The last submitted patch, 26: 2863262-26-BTB.patch, failed testing. View results

Anonymous’s picture

Status: Needs work » Needs review

Unrelated fails.

hi_ten_ja’s picture

Lendude’s picture

Status: Needs review » Needs work

@hiten2112 if you post a patch could you please comment on the changes you have made and why? Your patch only contains about 25% of the code of the patch in #26, so this doesn't look good.

Anonymous’s picture

A quick look at other works of @hiten2112 showed that similar questions he is asked in the all issues, and he did not consider it necessary to give any answers. So, let's just ignore #29.

At the moment, we have three approaches:

  1. #21 JTB version based on PhantomJS - super-minimal conversion, but we can not transfer it to Selenium as is.
  2. #25 BTB version with new trait CurlHelperTrait - maybe the trait has raw documentation, so if we choose this solution, we need to polish it once again.
  3. #26 BTB version with internal override of all necessary methods - it does not go beyond the scope of the convertible test, but it will not be useful if such mechanism are necessary for another test.
Lendude’s picture

I went through this again, and #26 makes the most sense to me. Lets not add a trait (and add more API we'd need to support) unless we are sure we need it.

I'm re-upping the patch from #26 just to make clear which part is under review.

dawehner’s picture

+++ b/core/tests/Drupal/FunctionalTests/Bootstrap/UncaughtExceptionTest.php
@@ -270,14 +268,108 @@ public function testLoggerException() {
+  protected function drupalGet($path, array $extra_options = [], array $headers = []) {
+    $url = $this->buildUrl($path, ['absolute' => TRUE]);
+
+    $ch = curl_init();
+    curl_setopt($ch, CURLOPT_URL, $url);
+    curl_setopt($ch, CURLOPT_HEADER, FALSE);
+    curl_setopt($ch, CURLOPT_RETURNTRANSFER, TRUE);
+    curl_setopt($ch, CURLOPT_USERAGENT, drupal_generate_test_ua($this->databasePrefix));
+    $this->response = curl_exec($ch);
+    $this->info = curl_getinfo($ch);
+    curl_close($ch);
+  }

Can't / should we use guzzle here?

Lendude’s picture

@dawehner yeah, I did a super quick trial with getHttpClient yesterday and it didn't work, but it would be much nicer if we could do that. I'll spend some more time on it today.

dawehner’s picture

+1, thank you for trying it.

Lendude’s picture

@dawehner++
@alexpott++

Now using Guzzle!

Lendude’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 36: 2863262-36.patch, failed testing. View results

Lendude’s picture

Status: Needs work » Needs review
FileSize
4.75 KB
13.69 KB

Turns out we can't use ServerException because it gets caught but not re-thrown by \Goutte\Client.

        // Let BrowserKit handle redirects
        try {
            $response = $this->getClient()->request($method, $uri, $requestOptions);
        } catch (RequestException $e) {
            $response = $e->getResponse();
            if (null === $response) {
                throw $e;
            }
        }

So now with a custom exception.

Lets see what this does.

Status: Needs review » Needs work

The last submitted patch, 39: 2863262-39.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Lendude’s picture

It works on my machine :(

Lendude’s picture

Status: Needs work » Needs review
FileSize
1.04 KB
13.6 KB

Just updated the comment, no idea why DrupalCI doesn't like this approach.

dawehner’s picture

I'll have a look at that if you don't mind.

Lendude’s picture

@dawehner go for it!

Status: Needs review » Needs work

The last submitted patch, 42: 2863262-42.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Lendude’s picture

Status: Needs work » Needs review
FileSize
1.25 KB
14.29 KB

Talked to @mixologic, he pointed to display_errors being off by default, so that might be a reason the request body is empty on DrupalCI. Lets see if this helps!

Status: Needs review » Needs work

The last submitted patch, 46: 2863262-46.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Lendude’s picture

Status: Needs work » Needs review
FileSize
7.04 KB
15.96 KB

no Guzzle, no cUrl. Lets see what this does.

And lets see if I got the drupalci trick right.

Status: Needs review » Needs work

The last submitted patch, 48: 2863262-48.patch, failed testing. View results

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

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Lendude’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
9.42 KB

per http://php.net/manual/en/errorfunc.configuration.php#ini.display-errors

Note:
Although display_errors may be set at runtime (with ini_set()), it won't have any effect if the script has fatal errors. This is because the desired runtime action does not get executed.

So might that be a reason that #48 doesn't work?

Strangely enough, if I switch off display_errors locally, the tests still pass locally, so the failures might not have anything to do with display_errors.

I'm throwing in the towel here. It works with cUrl, lets use cUrl. I'm reupping #26 again. I think that is as clean a conversion as we can get here.

Lendude’s picture

Status: Needs review » Reviewed & tested by the community

I'm going to RTBC this, since this is @vaplas' patch unchanged from #26.

I think all the work since #26 might be worth pursuing in a follow up, but I'd also feel fine with just figuring this is a pretty exceptional test case (see what I did there? :D) and we can live with it going outside the available APIs.

alexpott’s picture

Issue tags: +Needs followup

@Lendude I agree with doing further refactoring of UncaughtExceptionTest in a follow-up. I'm not a huge fan of the new assert* functions either. But it seems a good compromise to convert the text doing something a tiny bit hacky and then finesse in a follow-up. Can you create that before commit. Cheers!

jibran’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 90bf3dc227 to 8.7.x and 374f045ddb to 8.6.x. Thanks!

  • alexpott committed 90bf3dc on 8.7.x
    Issue #2863262 by Lendude, vaplas, Jo Fitzgerald, dawehner, alexpott:...

  • alexpott committed 374f045 on 8.6.x
    Issue #2863262 by Lendude, vaplas, Jo Fitzgerald, dawehner, alexpott:...

Status: Fixed » Closed (fixed)

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