Comments

nlisgo created an issue. See original summary.

nlisgo’s picture

Assigned: nlisgo » Unassigned
Status: Active » Needs review
FileSize
4.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

+++ 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

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

Component: tracker.module » toolbar.module
Jo Fitzgerald’s picture

Component: toolbar.module » tracker.module
FileSize
981 bytes
4.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.

Jo Fitzgerald’s picture

Component: tracker.module » toolbar.module

Not sure why the component changed. Now corrected.

Lendude’s picture

Status: Needs work » Needs review
FileSize
1.27 KB
4.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

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

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

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

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

michielnugter’s picture

Issue tags: +phpunit initiative

  • catch committed 477fbfd on 8.4.x
    Issue #2863597 by Lendude, Jo Fitzgerald, michielnugter, nlisgo,...
catch’s picture

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.