GitlabCI support: Fix all jobs validation errors and PHPUnit tests

Prashant.c → made their first commit to this issue’s fork.

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 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 one failure: 📌 When using drupalGet(), provide an associative array for $headers Needs work

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: 📌 GitlabCI: Fix ESLINT validation errors Needs review .

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 📌 When using drupalGet(), provide an associative array for $headers Needs work 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 testing in one go and getting CSpell, PHPCS, PHPStan and PHPUnit fixed would already be a great step forward.

The work on ESLint was moved to a separate issue, see: 📌 GitlabCI: Fix ESLINT validation errors Active , 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)

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/181112
 

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

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 📌 Add Drupal 11 support Needs work , please review.