GitlabCI support: Add config file and fix PHPUnit tests

Incorporated a .gitlab-ci.yml file into the project's root directory by utilizing the Drupal Association's template.gitlab-ci.yml contents. Continuously learning about it as I progress.

Thanks

Working on other warnings/errors.

Fixed all issue except eslint and PHPunit.
I would suggest to create a new issue for eslint warnings. Also, not sure why PHP unit cases are failing.
Thanks

Fixed one failing functional test by adding another permission so sub-menu-items would show up.

Not sure how we should deal with the remaining issue. AdminToolbarAdminMenuTest extends ToolbarAdminMenuTest. ToolbarAdminMenuTest (and a bunch of other tests in Core) use drupalGet() and set HTTP headers by sending them as a complete string ("header-name: header-value"), for example here. But drupalGet() expects the headers to be sent with the header name as the key and the value as, well, the value.

This should probably be addressed as a Core issue, no?

@marcoliver

Thanks for your efforts on this but should not the test failures be taken as a separate issue?

Some great work in here, thanks very much!

I'd like to recommend making eslint failures a follow-up issue since they're not required for a module to start using Gitlab CI, as such I've made that change to the template in the hopes that will be accepted.

I fixed one tiny phpcs issue, not sure why that came up when it passed earlier :shrug:

Some projects are adopting a more extensible approach to the template which is to only set the yml you need and point back the original for documentation which I would also like to advocate for, like this:

################
# DrupalCI GitLabCI template
#
# @see template for documentation https://git.drupalcode.org/project/gitlab_templates/-/blob/main/gitlab-ci/template.gitlab-ci.yml
################
include:
  - project: $_GITLAB_TEMPLATES_REPO
    ref: $_GITLAB_TEMPLATES_REF
    file:
      - "/includes/include.drupalci.main.yml"
      - "/includes/include.drupalci.variables.yml"
      - "/includes/include.drupalci.workflows.yml"
variables:  
  SKIP_ESLINT: '1'

I haven't made that change as it seemed too much of a departure from the status quo, but I'd be very happy to do so.

I've tried to figure out why the functional test is failing and... well, failed. There are some inconsistent failures which make me think the browser engine isn't keeping up with the test suite (race condition in JS) but there's one consistent test failure on AdminToolbarAdminMenuTest and that extends a core test which passes in the core test suite.

I think #10 is correct in stating that test failures aren't necessarily a blocker to switching to the new test system, especially when the main branch is failing in the old system anyway.

IMHO switching to Gitlab CI should just be a matter of adding and configuring .gitlab-ci.yml : fixing the tests could very well be done in followups, taking advantage of Gitlab, especially considering they didn't pass before either. Conflating the CI change with bug fixes is the reason why this has been waiting for 4 months.

This could unblock the D11 issue, and make it possibly easier to work on the various test issues.

Here's the cause of the most persistent failure: #3440169: When using drupalGet(), provide an associative array for $headers

Thanks @marcoliver ;)

Thanks a lot everyone for all the work on this issue.

One thing though: Why would ESLint be disabled?

I've taken a quick look at the issues raised by the ESLint job:
https://git.drupalcode.org/issue/admin_toolbar-3407845/-/jobs/1291671
They don't look very difficult to fix, especially since 171 errors out of 192 could be automatically fixed with the --fix flag.
(see report linked above)

If it's a question of work and amount of changes, if fixing the ESLint job would take too much work, we could leave it aside/untouched and create a different issue specifically to fix ESLint for the module, see for example: #3448470: GitlabCI: Fix ESLINT validation errors.

But I would certainly advise against completely shutting down the job, see:
https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/68/dif...

variables:
  SKIP_ESLINT: '1'

Switching back to Needs work, as an attempt to get feedback and perhaps an update to the GitlabCI file to revert the ESLint configuration.

Any feedback, comments, suggestions or questions would be greatly appreciated.

Feel free to let us know if you have any questions or concerns on any aspects of this comment or the ticket in general, we would surely be glad to help.
Thanks in advance!

Agree with #13 here - there's also no reason to disable the eslint job you can simply ignore the warnings (it does not fail the pipeline by default) - the same goes for phpcs and phpstan. IMO do the bare minimum to get this in and branch out into other issues. Unblocking D11 support would be fantastic.

Quick follow-up on this issue:

A few additional commits were added to the current merge request MR!68 with a few comments, see above.

But mostly, at this point:
Last build on MR!68: https://git.drupalcode.org/issue/admin_toolbar-3407845/-/pipelines/180699

• The ESLINT job now seems to be passing ✅
  ESLINT job: https://git.drupalcode.org/issue/admin_toolbar-3407845/-/jobs/1672678
   
• The PHPUnit Tests are passing as well 🟢 \o/
  PHPUnit job: https://git.drupalcode.org/issue/admin_toolbar-3407845/-/jobs/1672680

In order for the tests to pass, the broken core test \Drupal\Tests\toolbar\Functional\ToolbarAdminMenuTest::testSubtreesJsonRequest had to be temporarily disabled, until related issue #3440169: When using drupalGet(), provide an associative array for $headers is fixed. From a testing standpoint, this failing core test shouldn't be holding up the tests for the admin_toolbar module: Whenever it is fixed in core, it could be re-enabled for the module.
See: https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/68/dif...
 

At this point, all tests now seem to be passing green 🟢\o/ 🎉
https://git.drupalcode.org/issue/admin_toolbar-3407845/-/pipelines/180699
thus marking issue as Needs review and bumping as Major as an attempt to attract maintainers attention.

We would greatly appreciate if a maintainer or someone with write permission could take a look at ticket's merge request MR!68 and let us know if there would be any more work needed.

Feel free to let us know if you have any questions or concerns on merge request MR!68 or any aspect of this ticket in general, we would surely be glad to help.
Thanks in advance for your feedback and reviews.

Nice work getting everything green, but as previously stated eslint, stylelint, phpcs, and phpstan are NOT required to be green to get a passing pipeline. By default these fail with warnings. The MR as it currently stands has many changes unrelated to getting a functional pipeline - these could all be pushed into other issues, making it quicker for a maintainer to review and accept this issue. I think this is especially true for the CSS and JS changes as these will almost certainly require manual testing.

The previously stripped back gitlab ci file has now also been completely overwritten.

For what it's worth, I'm with @acbramley on this.

When I was contributing to this earlier I was yet to come across any kind of guidance on how to approach adopting GitLab CI, but senior people in the community all seem to be coalescing around "just do it and fix the lint failures later".

That said, the final decision on how to approach this is down to the maintainers of this module of course, who are yet to comment.

Thanks for the prompt feedback.

Agreed: seems too risky to include these changes in the same MR, sounds like a lot to test in one go and getting CSpell, PHPCS, PHPStan, PHPUnit and Stylelint fixed would already be a great step forward.

The work on ESLint was moved to a separate issue, see: #3449577: GitlabCI: Fix ESLINT validation errors, thus reverted all the changes to JS files and eslint configuration for Gitlab CI.
(Changes were mostly cherry-picked to be moved to a different branch/issue fork, see MR!75)

Required stylelint job to pass validation as well.

At this point, all tests now seem to be passing green 🟢 (Except ESLint, to be addressed in a separate merge request).
https://git.drupalcode.org/issue/admin_toolbar-3407845/-/pipelines/181141
 

Feel free to let us know if you have any questions or concerns on merge request MR!68 or any aspect of this ticket in general, we would surely be glad to help.
Thanks in advance for your feedback and reviews.

Reviewed the gitlab template ~~ All CI jobs for phpcs, phpstan and phpunit covered properly, Hence marking this one as RTBC

See MR comments

Thanks a lot Adam (@acbramley) for your prompt feedback and very constructive review once again, it's super appreciated 🙂

Not sure whether ticket's status should be changed back to Needs review, but the last comments have been answered.

Additional reviews, comments and feedback would be greatly appreciated.
Thanks in advance !

Commits are cherry-picked from this issue to #3363604: Add Drupal 11 support, please review.

Quick follow-up on this issue:

• Removed functional test method override since related issue #3440169: When using drupalGet(), provide an associative array for $headers was fixed in core toolbar tests.
• Disabled tests coverage for previous minor (10.2) due to BC issue. If we wanted 10.2 to be covered, we'd have to disable phpunit tests by default create test groups and enable testing and set test groups for each job. See for example:
  https://git.drupalcode.org/project/reroute_email/-/blob/2.3.x/.gitlab-ci...
• Fixed additional 'stylelint' errors ('prettier/prettier') in CSS files.
• Deprecated function 'toolbar_tools_menu_navigation_links' in favor of 'admin_toolbar_tools_menu_navigation_links'.

MR!68 is now passing all tests and jobs except ESLint.

Moving issue back to Needs review as an attempt to get more reviews and feedback.

Thanks in advance!

Not a maintainer, but if I were, I would ask that this is reduced to the changes that are actually required to enable CI and get it to pass in the default configuration. For example cspell, css changes and probably also php coding standards, at least the part that requires deprecations.

The last commit on this project was 9 month ago. Clearly the maintainer is currently not very active, and since this also blocks Drupal 11 compatibility at the moment, it should be kept as simple as possible for a maintainer to review and commit.

I also don't get the comments about 10.2 testing. That still worked in the previous MR and previous major testing also still works, what is the problem with 10.2? In my projects, I usually keep previous minor on, since I want to ensure that my projects work on supported core versions.

Yep, agree with #29, see #19 and #17

If I have time I'll try to spin an MR with just the relevant changes.

Thanks for the prompt reviews and feedback!

Created new merge request MR!83 with the bare minimum:

• GitlabCI template copied from DO-3440169.
• Fixed PHPUnit tests.

Everything else was removed.

New merge request MR!83 seems to pass PHPUnit tests, see last pipeline:
https://git.drupalcode.org/issue/admin_toolbar-3407845/-/pipelines/226057

Feel free to broaden test coverage as required.

Thanks in advance!

Thanks for working on this @DYdave and @acbramley! I agree with @berdir that focus should be on getting required tests to pass, since Drupal 11 release is close now, in about two weeks:

Week of July 29, 2024 (UTC) | Drupal 11.0.0 released.

https://www.drupal.org/about/core/policies/core-release-cycles/schedule#...

Ideally, a Drupal 11 ready release of Admin Toolbar should have been made a while ago ...

Thanks @ressa!

The requested changes above have been made to MR!83, which should at this point have the bare minimum changes to add GitlabCI support and get PHPUnit tests to pass.

Thanks in advance for your reviews and feedback.

Looks great, thanks @DYdave

Can we get it merged, Thanks!

Changes looks good, CI passing, can we get it merged? this will unblock the Drupal 11 compatibility issue - #3363604: Add Drupal 11 support.
Thanks!

Added gitlab and fixed tests already. One point though, I think the sort method in this test might actually fix #3465313: Max Bundles not being honored so moving that part of the commit there and closing this.

It's a shame everyone that worked on this didn't get credited tbh

Attempt to credit everyone on this issue as requested at #39.

Moving this to Fixed, as an attempt to credit everyone on this issue.

@acbramley:
Looks like the update worked this time 👍

The credit appears on your profile now.

Sorry again for the delay on this and we're certainly very grateful for all the work everyone produced to get this ticket through.

Please let us know if there is any other issue on which you should have been granted credit, we would surely be glad to take another look.
Thanks again everyone! 🙏

Yes, thank you @dydave and everyone else, who helped fix this issue!

Understanding how to assign credit for an issue can be a challenge, and I only really understood it after reading the documentation page several times and re-writing it.

Giving credit: Check and uncheck the names of users listed in the table, depending on whether they contributed to moving the issue towards resolution. The credits will appear on the user and organization profiles when the issue status is changed to Fixed, or when it automatically changes to Closed (fixed).

So this is great, to get confirmed that that's how it works -- since you probably checked the names, and saved ... but no-one actually got credited, until the issue was saved with Status "Fixed".

An alternative (for maintainers only) if you don't want to mark an issue as fixed because it was solved elsewhere - is to use the username search box in the crediting section of the solved issue to credit users from the first issue.

Marking it fixed is easier though :)