Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Comment | File | Size | Author |
---|---|---|---|
#11 | 2863597-11.patch | 4.67 KB | michielnugter |
#11 | interdiff-9-11.txt | 1.96 KB | michielnugter |
#9 | 2863597-9.patch | 4.69 KB | Lendude |
#9 | interdiff-2863597-6-9.txt | 1.27 KB | Lendude |
#6 | convert_web_tests_to-2863597-6.patch | 4.67 KB | jofitz |
Comments
Comment #2
nlisgo CreditAttribution: nlisgo commentedWhen 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:
I'm posting up my progress so far. And will wait for feedback or assistance.
Comment #3
dawehnerIt feels like this test should rather be converted to a
JavaScriptTestBase
. What do you think?Comment #4
nlisgo CreditAttribution: nlisgo commentedBtw, 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.
Comment #5
nlisgo CreditAttribution: nlisgo commentedComment #6
jofitz CreditAttribution: jofitz at ComputerMinds commentedConverted ToolbarAdminMenuTest to a JavascriptTestBase, this does not make any more changes than if it were BTB.
Comment #8
jofitz CreditAttribution: jofitz at ComputerMinds commentedNot sure why the component changed. Now corrected.
Comment #9
LendudeThis 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.
Comment #10
dawehner+1 for minimal changes. Can we though please open up the follow up so we don't forget about them?
Comment #11
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedI 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.
Comment #12
LendudeI 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.
Comment #13
dawehner#12 is some good argument.
Doers someone mind opening up a follow up to have some javascript test instead?
Comment #14
Lendudefollow up #2870128: Add javascript test coverage to supplement ToolbarAdminMenuTest
Comment #15
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedComment #17
catchCommitted/pushed to 8.4.x, thanks!