follow up to #2940679: Fix FilterIdTest test names

By converting the comments for each provider data set to the keys we can improve the error output. To quote larowlan

e.g from

     // The filter ID is mapped, and the plugin exists.
      [
        'foo',
        'filter_html',
      ],

to

      'filter ID mapped to plugin that exists' => [
        'foo',
        'filter_html',
      ],
CommentFileSizeAuthor
#12 interdiff_5-11.txt1.48 KBspitzialist
#11 core-improve-filtertest-provider-keys-2974657-11.patch1.63 KBspitzialist
#5 interdiff_3-5.txt708 bytesleolandotan
#5 core-improve-filtertest-provider-keys-2974657-5-D8.patch1.48 KBleolandotan
#3 core-improve-filtertest-provider-keys-D8.patch1.48 KBleolandotan

Comments

neclimdul created an issue. See original summary.

leolandotan’s picture

leolandotan commented
Assigned: Unassigned » leolandotan

I'll work on this.

leolandotan’s picture

leolandotan commented
Assigned: leolandotan » Unassigned
Status: Active » Needs review
StatusFileSize
new core-improve-filtertest-provider-keys-D8.patch1.48 KB

Hi guys,

I added the key for each provider data set based on the comment for each provider. I just didn't remove the comment yet since it can be clearer that the key.

Hope everything is in order.

Thanks!

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Needs work 
+++ b/core/modules/filter/tests/src/Kernel/Plugin/migrate/process/FilterIdTest.php
@@ -88,25 +88,25 @@ public function testTransform($value, $expected_value, $invalid_id = NULL) {
+      'filter ID mapped to plugin that does not exists' => [

/s/exists/exist/

leolandotan’s picture

leolandotan commented
Status: Needs work » Needs review
StatusFileSize
new core-improve-filtertest-provider-keys-2974657-5-D8.patch1.48 KB
new interdiff_3-5.txt708 bytes

Thank you for your review @borisson_! I have applied the changes.

Hope everything is in order.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Reviewed & tested by the community

Looks great!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Title: Impove FilterIdTest provider keys » Improve FilterIdTest provider keys
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/filter/tests/src/Kernel/Plugin/migrate/process/FilterIdTest.php
@@ -88,25 +88,25 @@ public function testTransform($value, $expected_value, $invalid_id = NULL) {
       // The filter ID is mapped, and the plugin exists.
...
       // The filter ID isn't mapped, but it's unchanged from the source (i.e.,
       // it bypasses the static map) and the plugin exists.
...
       // The filter ID is mapped, but the plugin does not exist.
...
       // The filter ID isn't mapped, but it's unchanged from the source (i.e.,
       // it bypasses the static map) but the plugin does not exist.

We can remove the comments now as they are redundant. Plus there is one test which does not have a comment or a key - we should fix that too. Maybe there's a comment on #2618332: Better handle replacement of missing filters with filter_null that will explain why this test is there.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

spitzialist’s picture

spitzialist commented
Assigned: Unassigned » spitzialist
spitzialist’s picture

spitzialist commented
Assigned: spitzialist » Unassigned
Status: Needs work » Needs review
StatusFileSize
new core-improve-filtertest-provider-keys-2974657-11.patch1.63 KB

Patch created to include changes proposed in comment #8:
- Removed comments
- Added key for last test case (Please review, as I am not sure here)

spitzialist’s picture

spitzialist commented
StatusFileSize
new interdiff_5-11.txt1.48 KB
borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Reviewed & tested by the community

This looks like an improvement over the current state.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Credited @neclimdul for creating the issue and myself for reviews.

Committed and pushed 5d8c1f4907 to 8.7.x and 71036622db to 8.6.x. Thanks!

Backported to 8.6.x since it is tests only.

  • alexpott committed 5d8c1f4 on 8.7.x 
    Issue #2974657 by leolando.tan, spitzialist, borisson_, neclimdul,...

  • alexpott committed 7103662 on 8.6.x 
    Issue #2974657 by leolando.tan, spitzialist, borisson_, neclimdul,...

Status: Fixed » Closed (fixed)

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