This ticket is a follow-up to #3407845: GitlabCI support: Add config file and fix PHPUnit tests.

Problem/Motivation

The last pipeline for the current development branch 3.x reported quite a few errors for the job ESLINT, see:

Steps to reproduce

Build > Pipeline > Run pipeline :
https://git.drupalcode.org/issue/admin_toolbar-3407845/-/pipelines
Select branch: 3407845-add-gitlab-ci

Proposed resolution

Fix all errors reported by ESLINT:

  • Apply automated correction of errors with the --fix flag.
  • Fix all other errors manually, based on suggestions.
  • Ignore certain files if necessary to be excluded from validation, such as external hoverIntent jQuery Plugin library.

If possible : Require eslint job to pass in Gitlab CI configuration.

Remaining tasks

Help testing merge request, see #11.
 

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

Issue fork admin_toolbar-3449577

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

DYdave created an issue. See original summary.

DYdave opened merge request !75

dydave’s picture

dydave commented
Status: Active » Needs review
Parent issue: » #3407845: GitlabCI support: Add config file and fix PHPUnit tests
Related issues: -#3407845: GitlabCI support: Add config file and fix PHPUnit tests

Quick follow-up on this issue:

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

But mostly, at this point:
Last build on MR!75: https://git.drupalcode.org/issue/admin_toolbar-3449577/-/pipelines/181273

 

Added temporary commit to be reverted:
https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/75/dif...
ported from #3407845: GitlabCI support: Add config file and fix PHPUnit tests, based on MR!68 to pass phpunit tests and add .gitlab-ci.yml file.
Revert this commit once these changes are merged in the development branch.
 

We would greatly appreciate if a maintainer or someone with write permission could take a look at ticket's merge request MR!75 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!75 or any aspect of this ticket in general, we would surely be glad to help.
Thanks in advance for your feedback and reviews.

rajeshreeputra’s picture

rajeshreeputra
he/him
Location Pune
commented
Related issues: +#3363604: Add Drupal 11 support
rajeshreeputra’s picture

rajeshreeputra
he/him
Location Pune
commented
Status: Needs review » Reviewed & tested by the community

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

dydave’s picture

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

Rebased MR!75 on 3.x with gitlabci tests and required eslint job to pass.

✅ Merge request seems to complete successfully with all ESLint validation errors fixed, see:

Back to Needs review for more reviews, testing and feedback.
Thanks!

japerry’s picture

japerry
Location KOMK
commented

Because our tests don't have great coverage, I'd want to ensure this is manually tested a bit before committing. I've had issues where seemingly innocent js linting changes actually broke modules before.

dydave’s picture

dydave commented
Title: GitlabCI: Fix ESLINT validation errors » GitlabCI: Fix ESLINT validation errors in admin_toolbar
Issue summary: View changes
Status: Needs review » Needs work

More work needed.

dydave opened merge request !116

dydave changed the visibility of the branch 3449577-gitlabci-fix-eslint to hidden.

dydave’s picture

dydave commented
Title: GitlabCI: Fix ESLINT validation errors in admin_toolbar » GitlabCI: Fix ESLINT validation errors
Priority: Normal » Major
Issue summary: View changes
Status: Needs work » Needs review

Re-rolled previous merge request for the latest version of the module, fixed all conflicts and created a new merge request:
https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/116

ESLINT job is passing green 🟢
Build: https://git.drupalcode.org/project/admin_toolbar/-/pipelines/431041
Job: https://git.drupalcode.org/project/admin_toolbar/-/jobs/4439405

Additionally, all PHPUnit tests and in particular FunctionalJavascript are all passing 🟢

I've done a bit of testing on a fresh local install, on D10.4 and everything seemed to work fine, as expected.
 

HELP TESTING:
If you would like to help on this ticket, could you please help testing merge request MR!116?

Impacted areas and what to test:

  • Admin Toolbar Search
  • Admin Toolbar Hover, Focus, Click, Keydown ==> Interacting with the Toolbar.

 
We would also appreciate code reviews of MR!116.

But mostly: Applied a round of automated fixes with the --fix flag and fixed the remaining errors manually.
Changes to fix the 'unnamed functions' errors are probably the most impacting.
 

Moving this to Major as an attempt to attract attention and get more help on this issue.
Enforcing tight JS coding standards in the code base would allow detecting and preventing potential errors being introduced in submitted merge requests.

Back to Needs review.

Thanks in advance!

dydave closed merge request !75

dydave closed merge request !116

dydave’s picture

dydave commented
Priority: Major » Normal
Status: Needs review » Fixed

Thanks again everyone on this issue!

Overall, it was too complicated to get all these changes reviewed, tested and merged at the same time, so the two JS files were broken down in two separate issues:

The changes have been merged and the ESLINT job is finally passing 🟢 \o/

Marking this issue as Fixed, for now. 🥳

Thanks again very much for the great help keeping the module well maintained! 🙏

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.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.