Add automated regression tests

Created on 1 March 2024, 9 months ago
Updated 17 April 2024, 7 months ago

Problem/Motivation

Now that we have GitLab CI running ( 📌 Set up GitLab CI Fixed ), we should add some automated regression tests!

This will help contributors to know if the patch they are proposing will something for someone else. It will also help maintainers make changes more confidently, including if the security team was to contact them .

Proposed resolution

Add automated tests for the CKEditor abbreviation functionality. They will likely look like the automated tests for the "link" functionality already present in CKEditor.

Remaining tasks

  1. (@mparker17 doesn't think this is needed)

User interface changes

None.

API changes

None.

Data model changes

None.

Feature request
Status

Fixed

Version

4.0

Component

Code

Created by

🇨🇦Canada mparker17 UTC-4

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @mparker17
  • 🇨🇦Canada mparker17 UTC-4

    Note that I've started on some tests in my local environment, but due to other factors limiting my time this week, I haven't committed them yet. I do plan to get back to them soon, however!

  • Status changed to Needs review 8 months ago
  • 🇨🇦Canada mparker17 UTC-4

    The tests in this merge request work on Drupal 9 and 10, testing CKEditor5.

    More specifically, it covers the CKEditor Abbreviation JS library, and Drupal's integration with it through the text format, toolbar, and custom integration CSS/JS.

    I consider this ready for review! Feedback is welcome.

    Given that Drupal 9 has reached end of life on November 1st, 2023 , I haven't bothered to write tests for Drupal 9 testing CKEditor4, i.e.: the file at web/modules/contrib/ckeditor_abbreviation/src/Plugin/CKEditorPlugin/AbbreviationCKEditorButton.php remains untested. Note that we can't drop support for D9, delete the CKEditor4 plugin without breaking backwards compatibility, so deleting this will have to wait for major version 5.

    Also, regarding the ckeditor_abbreviation.abbreviation_dialog route, and the form it points to (\Drupal\ckeditor_abbreviation\Form\CkeditorAbbreviationDialog)... I can't figure out where these are used, so I'm not sure how to test them, and they remain untested. I suspect this is "dead code", i.e.: no longer used... but if that's wrong, please let me know how I could test this code!

  • Status changed to Needs work 8 months ago
  • 🇨🇦Canada mparker17 UTC-4

    I should add a test to check that the title attribute is left out if it is empty, i.e.: functionality added in commit 27c25c99.

  • Status changed to Needs review 8 months ago
  • 🇨🇦Canada mparker17 UTC-4

    Awesome, new tests pass with no linter notices, so this is ready for review again.

  • 🇨🇦Canada mparker17 UTC-4

    Was trying to add a test for editing an abbreviation, but that turned out to be more difficult than it appeared, because the API for selecting text in JavaScript acts differently depending on whether you happen to be looking at a Document Node containing only text, or a Document Node containing text mixed with HTML elements. CKEditor appears to have its own way to select text, but there isn't any good documentation on how to use it.

    Anyway, I think tests for editing an abbreviation can come later: they shouldn't block merging this.

  • Status changed to Fixed 8 months ago
  • 🇨🇦Canada mparker17 UTC-4

    After turning off the computer yesterday, I thought of an easier way to write a test to edit an abbreviation, and happily, it works.

    All the tests and lints are passing, so I'm going to merge it!

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

Production build 0.71.5 2024