Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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).
Comment | File | Size | Author |
---|---|---|---|
#51 | 2863262-26-BTB.patch | 9.42 KB | Lendude |
#48 | 2863262-48.patch | 15.96 KB | Lendude |
| |||
#48 | interdiff-2863262-46-48.txt | 7.04 KB | Lendude |
#46 | 2863262-46.patch | 14.29 KB | Lendude |
#46 | interdiff-2863262-42-46.txt | 1.25 KB | Lendude |
Comments
Comment #2
jofitz CreditAttribution: jofitz at ComputerMinds commentedComment #3
dawehner@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.Comment #4
dawehnerThe 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.
Comment #5
dawehnerKlausi 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 toDrupal\FunctionalTests\Bootstrap
Sorry I didn't take that into account before properly.
Comment #6
jofitz CreditAttribution: jofitz at ComputerMinds commentedMoved the files.
Comment #7
dawehnerThank you @Jo Fitzgerald!
Comment #8
alexpottShouldn't we also be making \Drupal\system\Tests\System\UncaughtExceptionTest a BrowserTestBase test in this issue too?
Comment #9
dawehner@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.
Comment #10
alexpott@dawehner basically I don't get why we'd move the fixtures before move the test that uses. They seem to belong together - no?
Comment #11
dawehnerFair, I see what you mean. Yeah let's keep the stuff around for now.
Comment #12
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedComment #13
dawehner@Jo Fitzgerald
Do you need help to solve this issue?
Comment #14
jofitz CreditAttribution: jofitz at ComputerMinds commented@dawehner What was the conclusion of your discussing with @alexpott? Do we need to move
UncaughtExceptionTest
?Comment #15
dawehner@Jo Fitzgerald
There are two possibilities:
UncaughtExceptionTest
and move the fixtures to coreUncaughtExceptionTest
, dont' move the fixtures, but then also don't convert the tests which needs these fixturesWhat do you think? I'd argue the first one seems a bit more reasonable.
Comment #16
jofitz CreditAttribution: jofitz at ComputerMinds commented@dawehner I think I'll step back from this issue, I'm getting more and more confused. Sorry.
Comment #17
dawehner@Jo Fitzgerald
Which parts do you not understand?
Comment #18
jofitz CreditAttribution: jofitz at ComputerMinds commented@dawehner There are a few things, including:
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?Perhaps we could have a chat on IRC at some point?
Comment #19
dawehnerSure, 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 :)
Comment #21
LendudeUncaughtExceptionTest
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.
Comment #25
Anonymous (not verified) CreditAttribution: Anonymous commentedAbsolutely! Art from @znerol! Exquisite work!
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):
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 intoassertions
. Therefore, you can set any value to $message, and WTB-test already will pass.Example
Instead of two asserts:
We can do one asserts:
$this->assertSame('Oh oh, flying teapots', $this->curlGetResponse())
2. The logic with
foreach (assertions)
andunset($assertion)
also necessary only for pass WTB-test, but not for BTB-test.Comment #26
Anonymous (not verified) CreditAttribution: Anonymous commentedAnother approach is to add all the necessary methods inside the
UncaughtExceptionTest
.Comment #28
Anonymous (not verified) CreditAttribution: Anonymous commentedUnrelated fails.
Comment #29
hi_ten_ja CreditAttribution: hi_ten_ja commentedComment #30
Lendude@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.
Comment #31
Anonymous (not verified) CreditAttribution: Anonymous commentedA 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:
CurlHelperTrait
- maybe the trait has raw documentation, so if we choose this solution, we need to polish it once again.Comment #32
LendudeI 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.
Comment #33
dawehnerCan't / should we use guzzle here?
Comment #34
Lendude@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.
Comment #35
dawehner+1, thank you for trying it.
Comment #36
Lendude@dawehner++
@alexpott++
Now using Guzzle!
Comment #37
LendudeComment #39
LendudeTurns out we can't use ServerException because it gets caught but not re-thrown by
\Goutte\Client
.So now with a custom exception.
Lets see what this does.
Comment #41
LendudeIt works on my machine :(
Comment #42
LendudeJust updated the comment, no idea why DrupalCI doesn't like this approach.
Comment #43
dawehnerI'll have a look at that if you don't mind.
Comment #44
Lendude@dawehner go for it!
Comment #46
LendudeTalked 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!
Comment #48
Lendudeno Guzzle, no cUrl. Lets see what this does.
And lets see if I got the drupalci trick right.
Comment #51
Lendudeper http://php.net/manual/en/errorfunc.configuration.php#ini.display-errors
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.
Comment #52
LendudeI'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.
Comment #53
alexpott@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!
Comment #54
jibranAdded #2987980: Refactor UncaughtExceptionTest to not use cUrl
Comment #55
alexpottCommitted and pushed 90bf3dc227 to 8.7.x and 374f045ddb to 8.6.x. Thanks!