See #2735005: Convert all Simpletest web tests to BrowserTestBase (or UnitTestBase/KernelTestBase)

CommentFileSizeAuthor
#11 2863597-11.patch4.67 KBmichielnugter
#11 interdiff-9-11.txt1.96 KBmichielnugter
#9 2863597-9.patch4.69 KBLendude
#9 interdiff-2863597-6-9.txt1.27 KBLendude
#6 convert_web_tests_to-2863597-6.patch4.67 KBjofitz
#6 interdiff-2-6.txt981 bytesjofitz
#2 convert_web_tests_to-2863597-2.patch4.61 KBnlisgo
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nlisgo created an issue. See original summary.

nlisgo’s picture

nlisgo CreditAttribution: nlisgo commented
Assigned: nlisgo » Unassigned
Status: Active » Needs review
FileSize
convert_web_tests_to-2863597-2.patch4.61 KB

When I moved the tests over to Drupal\Tests\toolbar\Functional there were 4 fails. I have fixed 3 of them but I am struggling to fix: Drupal\Tests\toolbar\Functional\ToolbarAdminMenuTest::testMenuLinkUpdateSubtreesHashCacheClear

The output is:

User error: Redirects to external URLs are not allowed by default, use \Drupal\Core\Routing\TrustedRedirectResponse for it.
Drupal\Core\EventSubscriber\RedirectResponseSubscriber->checkRedirectUrl()() (Line: 85)

 /Users/nlisgo/Sites/sandbox/drupal_contrib/web/core/lib/Drupal/Core/Test/HttpClientMiddleware/TestHttpClientMiddleware.php:44
 /Users/nlisgo/Sites/sandbox/drupal_contrib/web/vendor/guzzlehttp/promises/src/Promise.php:203
 /Users/nlisgo/Sites/sandbox/drupal_contrib/web/vendor/guzzlehttp/promises/src/Promise.php:156
 /Users/nlisgo/Sites/sandbox/drupal_contrib/web/vendor/guzzlehttp/promises/src/TaskQueue.php:61
 /Users/nlisgo/Sites/sandbox/drupal_contrib/web/vendor/guzzlehttp/promises/src/Promise.php:246
 /Users/nlisgo/Sites/sandbox/drupal_contrib/web/vendor/guzzlehttp/promises/src/Promise.php:223
 /Users/nlisgo/Sites/sandbox/drupal_contrib/web/vendor/guzzlehttp/promises/src/Promise.php:266
 /Users/nlisgo/Sites/sandbox/drupal_contrib/web/vendor/guzzlehttp/promises/src/Promise.php:225
 /Users/nlisgo/Sites/sandbox/drupal_contrib/web/vendor/guzzlehttp/promises/src/Promise.php:62
 /Users/nlisgo/Sites/sandbox/drupal_contrib/web/vendor/guzzlehttp/guzzle/src/Client.php:129
 /Users/nlisgo/Sites/sandbox/drupal_contrib/web/vendor/fabpot/goutte/Goutte/Client.php:138
 /Users/nlisgo/Sites/sandbox/drupal_contrib/web/vendor/symfony/browser-kit/Client.php:315
 /Users/nlisgo/Sites/sandbox/drupal_contrib/web/vendor/symfony/browser-kit/Client.php:262
 /Users/nlisgo/Sites/sandbox/drupal_contrib/web/vendor/behat/mink-browserkit-driver/src/BrowserKitDriver.php:696
 /Users/nlisgo/Sites/sandbox/drupal_contrib/web/vendor/behat/mink-browserkit-driver/src/BrowserKitDriver.php:480
 /Users/nlisgo/Sites/sandbox/drupal_contrib/web/vendor/behat/mink/src/Element/NodeElement.php:153
 /Users/nlisgo/Sites/sandbox/drupal_contrib/web/vendor/behat/mink/src/Element/NodeElement.php:161
 /Users/nlisgo/Sites/sandbox/drupal_contrib/web/core/tests/Drupal/Tests/BrowserTestBase.php:808
 /Users/nlisgo/Sites/sandbox/drupal_contrib/web/core/tests/Drupal/Tests/BrowserTestBase.php:912
 /Users/nlisgo/Sites/sandbox/drupal_contrib/web/core/modules/toolbar/tests/src/Functional/ToolbarAdminMenuTest.php:135

I'm posting up my progress so far. And will wait for feedback or assistance.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented 
+++ b/core/modules/toolbar/tests/src/Functional/ToolbarAdminMenuTest.php
@@ -330,7 +331,8 @@ public function testSubtreesJsonRequest() {
-    $ajax_result = $this->drupalGetAjax('toolbar/subtrees/' . $subtrees_hash);
+    $ajax_result = $this->drupalGet('toolbar/subtrees/' . $subtrees_hash, ['query' => [MainContentViewSubscriber::WRAPPER_FORMAT => 'drupal_ajax']]);
+    $ajax_result = json_decode($ajax_result, TRUE);

It feels like this test should rather be converted to a JavaScriptTestBase. What do you think?

nlisgo’s picture

nlisgo CreditAttribution: nlisgo commented

Btw, I have no idea how this patch went green. I was expecting to see one fail as indicated in my comment #2. I will see if there is something in my setup that could be causing it.

@dawehner I was given the instruction to change as few things as possible. The drupalGetAjax method is not available in Drupal\Tests\toolbar\Functional\BrowserTestBase. I believe that approach I have switched to is the closest to the current behaviour. If converting this test to JavaScriptTestBase is appropriate as a followup issue then I would favour that. Please advise.

nlisgo’s picture

nlisgo CreditAttribution: nlisgo commented
Component: tracker.module » toolbar.module
jofitz’s picture

jofitz CreditAttribution: jofitz at ComputerMinds commented
Component: toolbar.module » tracker.module
FileSize
interdiff-2-6.txt981 bytes
convert_web_tests_to-2863597-6.patch4.67 KB

Converted ToolbarAdminMenuTest to a JavascriptTestBase, this does not make any more changes than if it were BTB.

Status: Needs review » Needs work

The last submitted patch, 6: convert_web_tests_to-2863597-6.patch, failed testing.

jofitz’s picture

jofitz CreditAttribution: jofitz at ComputerMinds commented
Component: tracker.module » toolbar.module

Not sure why the component changed. Now corrected.

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude at Dx Experts commented
Status: Needs work » Needs review
FileSize
interdiff-2863597-6-9.txt1.27 KB
2863597-9.patch4.69 KB

This makes this green locally.

Turning ToolbarAdminMenuTest into a proper javascript test (that actually utilises all the javascript stuff and not just extends that base class) feels like follow up work, lets keep this as minimal as possible.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented

Turning ToolbarAdminMenuTest into a proper javascript test (that actually utilises all the javascript stuff and not just extends that base class) feels like follow up work, lets keep this as minimal as possible.

+1 for minimal changes. Can we though please open up the follow up so we don't forget about them?

michielnugter’s picture

michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commented
FileSize
interdiff-9-11.txt1.96 KB
2863597-11.patch4.67 KB

I got it to pass without converting the ToolbarAdminMenuTest to a Javascript test in this patch.

I agree that if any test tests something javascript it should be a JTB but converting it in this issue seems out of scope. We could open a follow-up but is it really a Javascript test or is it asserting a serverside behavior? I'm leaning more towards the last one, should it not be a Kernel test? Not really familiar enough with those tests to be sure.

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude at Dx Experts commented

but is it really a Javascript test or is it asserting a serverside behavior

I think it's doing the latter (Tests the caching of the admin menu subtree items.), but it's using $this->getDrupalSettings(); and a hash set there to do this. So to me, it looks like it's using a javascript setting to assert serverside behaviour, but I don't really see a great up side to using proper javascript over $this->getDrupalSettings() within BrowserTestBase. The javascript setting doesn't change because of any javascript action, but only on page reload anyway, so there doesn't seem to be any need for any actual javascript action to properly test this. It would only be about reading the setting using javascript instead of parsing it from the raw page (which I think happens for getDrupalSettings)

In short, I think #11 is fine, and I think ToolbarAdminMenuTest is valid in its current state.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented
Status: Needs review » Reviewed & tested by the community

#12 is some good argument.

Doers someone mind opening up a follow up to have some javascript test instead?

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude at Dx Experts commented

follow up #2870128: Add javascript test coverage to supplement ToolbarAdminMenuTest

michielnugter’s picture

michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commented
Issue tags: +phpunit initiative

  • catch committed 477fbfd on 8.4.x 
    Issue #2863597 by Lendude, Jo Fitzgerald, michielnugter, nlisgo,...
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 8.4.x, thanks!

Status: Fixed » Closed (fixed)

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