ResponsiveImageMappingEntityTest is assigned to incorrect groups.

In the docblock, groups Drupal and Config are defined.
In getInfo(), Picture is defined.

The getInfo group should be changed to Responsive Image to match ResponsiveImageAdminUITest and ResponsiveImageFieldDisplayTest (as per #2124377: Rename "Picture" module to "Responsive Image" module).

For consistency, the docblock groups should therefore be Drupal and Responsive_image.

CommentFileSizeAuthor
#6 fix_naming_in-2225677-5.patch9.37 KBEli-T
#2 2225677-ResponsiveImageMappingEntityTest-naming.patch9.37 KBmcjim
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Eli-T’s picture

Eli-T
he/him
Primary language English
Location Manchester
CreditAttribution: Eli-T commented
Title: Fix incorrect group names in ResponsiveImageMappingEntityTest » Fix naming in ResponsiveImageMappingEntityTest following module rename from Picture to Responsive Image
Related issues: +#2124377: Rename "Picture" module to "Responsive Image" module

We should also pull out all other references to Picture module in the unit tests out in this issue.

  • The class is called ResponsiveImageMappingEntityTest but is in a file PictureMappingEntityTest.php.
  • It has namespace Drupal\picture\Tests;
  • @coversDefaultClass refers to non-existent class \Drupal\picture\Entity\PictureMapping
  • Some of the variable names are now confusing, eg $picture_mapping = new ResponsiveImageMapping(array(), $this->entityTypeId);
mcjim’s picture

mcjim CreditAttribution: mcjim commented
Status: Active » Needs review
FileSize
2225677-ResponsiveImageMappingEntityTest-naming.patch9.37 KB

Patch does the following:

  • renames the file to ResponsiveImageMappingEntityTest.php
  • changes the namespace to Drupal\responsive_image\Tests
  • @coversDefaultClass now refers to \Drupal\responsive_image\Entity\ResponsiveImageMapping
  • changes variable $picture_mapping to $responsive_image_mapping
  • changes the docblock group Config to Responsive_image
  • changes the getInfo group from Picture to Responsive Image
Eli-T’s picture

Eli-T
he/him
Primary language English
Location Manchester
CreditAttribution: Eli-T commented
Related issues: +#2030653: Expand ResponsiveImageMapping with methods

Looks good and tests pass, but probably needs someone other than me to RTBC given I've already input in to the solution.

Note this will need reroll if #2030653: Expand ResponsiveImageMapping with methods is committed before it.

Eli-T’s picture

Eli-T
he/him
Primary language English
Location Manchester
CreditAttribution: Eli-T commented
Status: Needs review » Needs work

Needs slight reroll now #2030653: Expand ResponsiveImageMapping with methods *has* been committed.

Eli-T’s picture

Eli-T
he/him
Primary language English
Location Manchester
CreditAttribution: Eli-T commented
Assigned: Unassigned » Eli-T
Eli-T’s picture

Eli-T
he/him
Primary language English
Location Manchester
CreditAttribution: Eli-T commented
Assigned: Eli-T » Unassigned
Status: Needs work » Needs review
FileSize
fix_naming_in-2225677-5.patch9.37 KB

Reroll following #2225677: Fix naming in ResponsiveImageMappingEntityTest following module rename from Picture to Responsive Image

Status: Needs review » Needs work

The last submitted patch, 6: fix_naming_in-2225677-5.patch, failed testing.

Eli-T’s picture

Eli-T
he/him
Primary language English
Location Manchester
CreditAttribution: Eli-T commented
Status: Needs work » Needs review

6: fix_naming_in-2225677-5.patch queued for re-testing.

Eli-T’s picture

Eli-T
he/him
Primary language English
Location Manchester
CreditAttribution: Eli-T commented

Re-queued as all tests pass locally.

marcvangend’s picture

marcvangend
he/him
Primary language Dutch
Location Amsterdam
CreditAttribution: marcvangend commented
Status: Needs review » Reviewed & tested by the community

Code looks good, tests still pass locally, and all references to "picture" are gone. RTBC as far as I can tell.

alexpott’s picture

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

Committed 86a9e55 and pushed to 8.x. Thanks!

  • Commit 86a9e55 on 8.x by alexpott: 
    Issue #2225677 by Eli-T, mcjim: Fix naming in...

Status: Fixed » Closed (fixed)

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