Problem/Motivation

Move tests of Toolbar from other modules to the toolbar module.

Steps to reproduce

Proposed resolution

Aside from the moved tests this includes changing AlertsJsonFeedTest.php from a functionaljavascript test to a functional test.

  • core/modules/announcements_feed/tests/src/Functional/AlertsJsonFeedTest.php
  • core/modules/toolbar/tests/src/FunctionalJavascript/announcements_feeds/AccessAnnouncementTest.php
  • core/modules/toolbar/tests/src/FunctionalJavascript/ckeditor5/CKEditor5ToolbarTest.php
  • core/modules/toolbar/tests/src/FunctionalJavascript/layout_builder/LayoutBuilderToolbarTest.php
  • core/modules/toolbar/tests/src/FunctionalJavascript/workspaces_ui/WorkspaceToolbarIntegrationTest.php
  • core/modules/toolbar/tests/src/Unit/user/ToolbarLinkBuilderTest.php

Remaining tasks

Review

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3611750

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

quietone created an issue. See original summary.

quietone opened merge request !16353

quietone’s picture

quietone commented
Issue summary: View changes
Status: Active » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Disclosure used AI to come up with a grep

grep -rlE "protected( static)? \\\$modules" core/modules --include="*Test.php" \
    | xargs grep -lE "['\"]toolbar['\"]" 2>/dev/null \
    | grep -v "/toolbar/" \
    | while read f; do
        perl -0777 -ne 'exit(0) if /\$modules\s*=\s*\[[^\]]*[\x27"]toolbar[\x27"][^\]]*\]/s; exit(1)' "$f" \
          && echo "$f"
      done

And found a few more tests outside the toolbar module

core/modules/config_translation/tests/src/Functional/ConfigTranslationListUiTest.php
core/modules/shortcut/tests/src/Functional/ShortcutTranslationUITest.php
core/modules/shortcut/tests/src/Functional/ConfigTranslationListUiTest.php
core/modules/shortcut/tests/src/Functional/ShortcutCacheTagsTest.php
core/modules/shortcut/tests/src/Kernel/ShortcutCacheTagsTest.php
core/modules/contextual/tests/src/FunctionalJavascript/EditModeTest.php
core/modules/workspaces/tests/src/Functional/WorkspaceTest.php
core/modules/workspaces/tests/src/Functional/WorkspaceSwitcherTest.php
core/modules/system/tests/src/Functional/Theme/ToolbarClaroOverridesTest.php
core/modules/system/tests/src/Functional/Menu/MenuAccessTest.php
core/modules/announcements_feed/tests/src/Functional/AnnouncementsCacheTest.php
core/modules/announcements_feed/tests/src/Kernel/AnnounceFetcherUserTest.php

Assuming shortcut we can ignore as it's being deprecated too but the others seem valid

quietone’s picture

quietone commented
Title: Move toolbar integration tests to toolbar module » Move toolbar integration tests to toolbar module, part 1
Issue summary: View changes
Status: Needs work » Needs review

@smustgrave, thanks for looking at this.

The search above isn't going to provide the results we need. For two reasons that I can see, 1) they are limited to 'toolbar' in the $modules parameter and 2) they do not take into account the other issue, #3611749: Remove toolbar from tests where it is not needed.

After apply the diff from this issue and #3611749: Remove toolbar from tests where it is not needed. I started searching for "'toolbar'", using grep. Initially, modules such as jsonapi were not excluded. It was only added after checking the use of "'toolbar'" in any tests from the module. After all the checking, the result is 

$ git grep -lwi "'toolbar'" | grep -v core/modules/toolbar/ | grep -v core/modules/editor | grep -v core/modules/jsonapi | grep -v core/modules/shortcut |  grep -v core/modules/ckeditor5 | grep -v core/assets | grep -v phpstan-baseline | grep -v MAINTAINERS | grep Test.php | nl
     1  core/modules/contextual/tests/src/FunctionalJavascript/ContextualLinksTest.php
     2  core/modules/system/tests/src/Functional/Theme/ToolbarClaroOverridesTest.php
     3  core/modules/workspaces/tests/src/Functional/WorkspaceSwitcherTest.php
     4  core/modules/workspaces/tests/src/Functional/WorkspaceTest.php
     5  core/profiles/demo_umami/tests/src/FunctionalJavascript/OpenTelemetryAuthenticatedPerformanceTest.php

Those tests will require more work so will be done in a separate issue.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Ah my mistake! That MR has been merged so I applied again and ran the same grep and got the same results as you did in #5 so believe this one is good.

catch’s picture

catch
he/him
Primary language English
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to main and cherry-picked to 11.x, thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

catch closed merge request !16353

  • catch committed e70f323f on 11.x 
    task: #3611750 Move toolbar integration tests to toolbar module, part 1...

  • catch committed c1b93753 on main 
    task: #3611750 Move toolbar integration tests to toolbar module, part 1...
quietone’s picture

quietone commented
Status: Fixed » Needs work

This is causing test failures on 11.x, https://git.drupalcode.org/project/drupal/-/pipelines/897597/test_report...

  • catch committed ff4d7f90 on main 
    task: #3611750 Move toolbar integration tests to toolbar module, part 1...

  • catch committed 1b4aa278 on 11.x 
    task: #3611750 Move toolbar integration tests to toolbar module, part 1...
catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Fixed

There was a mistake in the test class namespace - pushed a commit to both branches to fix this.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • catch committed a6ff2282 on 11.x 
    task: #3611750 Move toolbar integration tests to toolbar module, part 1...

  • catch committed d1f2630e on main 
    task: #3611750 Move toolbar integration tests to toolbar module, part 1...

quietone reopened merge request !16353

quietone opened merge request !16394

quietone’s picture

quietone commented
Status: Fixed » Reviewed & tested by the community

So, I has incorrect namespaces in 5 tests files, including an incorrect directory name. The test that should have detected those mistakes did not and I made an issue for that, #3612498: PhpUnitApiFindAllClassFilesTest not finding namespace errors.

I tried to revert but had problems locally because of recently reinstalling Debian stable. While i was doing that catch made a fix on main, then another. We finally connected in Slack which resulted in updated MR for main and a new MR for 11.x. The 11.x one is passing tests, so that is promising.

I compared the two diffs and they agree, and tests are passing on 11.x so I am setting this back to RTBC.

quietone’s picture

quietone commented
Version: 11.x-dev » main

catch closed merge request !16353

catch closed merge request !16394

  • catch committed b259cc3a on 11.x 
    task: #3611750 Move toolbar integration tests to toolbar module, part 1...

  • catch committed c68bbb95 on main 
    task: #3611750 Move toolbar integration tests to toolbar module, part 1...
catch’s picture

catch
he/him
Primary language English
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to main and 11.x, thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.