Add test coverage for Help text displaying when editing an image effect

I manually tested and is fixed now in D8. Moving back to D7.

We'll need to add an automated test for this. See the instructions for adding automated tests.

Also, I haven't looked into the code, but I'm guessing the other $arg[] indexes in the line changed by the patch will also need to be updated.

<ul>
  <li>error: patch failed: core/modules/image/image.module:64</li>
  <li>error: core/modules/image/image.module: patch does not apply</li>
</ul>

This patch is out-dated. Needs re-roll.

I'm currently working on the automated tests for this regression.

This patch contains the automated tests for this issue.

Here's a new patch for D7.

Added patch that contains both the automated tests + the patch that will fix the issue.

Committed to 7.x - thanks!

Fixed a lot of small things in the tests on commit:

diff --git a/modules/image/image.test b/modules/image/image.test
index fffd7db..87d803a 100644
--- a/modules/image/image.test
+++ b/modules/image/image.test
@@ -491,8 +491,6 @@ class ImageEffectsUnitTest extends ImageToolkitTestCase {
  * Tests the administrative user interface.
  */
 class ImageAdminUiTestCase extends ImageFieldTestCase {
-
-
   public static function getInfo() {
     return array(
       'name' => 'Administrative user interface',
@@ -507,8 +505,6 @@ class ImageAdminUiTestCase extends ImageFieldTestCase {
 
   /**
    * Test if the help text is available on the add effect form.
-   *
-   * This is a regression test for https://www.drupal.org/node/1737714.
    */
   function testAddEffectHelpText() {
     // Create a random image style.
@@ -516,31 +512,29 @@ class ImageAdminUiTestCase extends ImageFieldTestCase {
 
     // Open the add effect form and check for the help text.
     $this->drupalGet($style['path'] . '/add/image_crop');
-    $this->assertText(t('Cropping will remove portions of an image to make it the specified dimensions.'), 'The Image Style effect help text was displayed on the add effect page.');
+    $this->assertText(t('Cropping will remove portions of an image to make it the specified dimensions.'), 'The image style effect help text was displayed on the add effect page.');
   }
 
   /**
    * Test if the help text is available on the edit effect form.
-   *
-   * This is a regression test for https://www.drupal.org/node/1737714.
    */
   function testEditEffectHelpText() {
     // Create a random image style.
-    $rand_style = $this->createRandomStyle();
+    $random_style = $this->createRandomStyle();
 
     // Add the crop effect to the image style.
     $edit = array();
     $edit['data[width]'] = 20;
     $edit['data[height]'] = 20;
-    $this->drupalPost($rand_style['path'] . '/add/image_crop', $edit, t('Add effect'));
+    $this->drupalPost($random_style['path'] . '/add/image_crop', $edit, t('Add effect'));
 
     // Open the edit effect form and check for the help text.
     drupal_static_reset('image_styles');
-    $style = image_style_load($rand_style['name']);
+    $style = image_style_load($random_style['name']);
 
     foreach ($style['effects'] as $ieid => $effect) {
-      $this->drupalGet($rand_style['path'] . '/effects/' . $ieid);
-      $this->assertText(t('Cropping will remove portions of an image to make it the specified dimensions.'), 'The Image Style effect help text was displayed on the edit effect page.');
+      $this->drupalGet($random_style['path'] . '/effects/' . $ieid);
+      $this->assertText(t('Cropping will remove portions of an image to make it the specified dimensions.'), 'The image style effect help text was displayed on the edit effect page.');
     }
   }
 }

@David_Rothstein,

Does this applies also to D8 ? Should we move this issue into a D8 version ?

Actually those tests look pretty nice and I doubt there's any tests like that in Drupal 8 now, so it would be good to forward-port them.

Does this applies also to D8 ? Should we move this issue into a D8 version ?

Oh, I guess I cross-posted :) Based on earlier comments and what I see in the admin UI, the bug itself doesn't affect Drupal 8, so just the tests would be needed.

@David_Rothstein, only the test or the entire patch ?

Just the tests... but if they fail because I'm wrong, then the entire patch :)

Patch ported for 8.0.x, but seems to fail as it is not loading the help text.

The issue doesn't affect 8.0.x but the help text is not displayed when running tests.

This may help going further in the test

Just added documentation to public static $modules = array('help', 'block', 'system');

The new development version is 8.2

Patch could not be applied to 8.2.x branch. Rerolling the patch for 8.2.x.

I think that we can apply this against 8.1.x. Check out the Allowed changes during the Drupal 8 release cycle. There's nothing specifically in that document about this case, probably because it's unusual to be forward-porting anything. It's a non-disruptive change though. We're not even fixing a bug here, much less changing an API or altering the UI. If anything, it would be helpful to include these tests in the earliest minor version possible to help ensure its stability moving forward.

#28 applies to 8.1.x so there's no reason to set the status to Needs Work.

I did a code review of this #28 and found a few problems. A new patch needs to be rolled with the following changes.

1. +++ b/core/modules/image/src/Tests/ImageAdminStylesTest.php
   @@ -332,10 +332,7 @@ public function testAjaxEnabledEffectForm() {
   +    list($style, $style_name, ) = $this->createRandomStyle();
   

   The final comma and space inside the list() call can be deleted.

2. +++ b/core/modules/image/src/Tests/ImageAdminUiTest.php
   @@ -0,0 +1,67 @@
   +    // Open the edit effect form and check fImagor the help text.
   

   There's a typo in this comment.

3. +++ b/core/modules/image/src/Tests/ImageAdminUiTest.php
   @@ -0,0 +1,67 @@
   +      $this->assertText(t('Resizing will make images an exact set of dimensions. This may cause images to be stretched or shrunk disproportionately.'), 'The image style effect help text was displayed on the add effect page.');
   

   The assert message needs to be edited, "...on the edit effect page."

Otherwise, the patch looks good and I'd RTBC it.

I applied the changes from #30.

Patch in #31 apply fine for me on 8.1.x branch. Looks good to me.

#31 is RTBC. The problems that I mentioned in #30 have been corrected. The tests are functionally the same as those that were committed to D7.

Good work today in NOLA @afemath and @zipymonkey!

Reran the test on parch #31 since the failure was to be in core/modules/migrate_drupal_ui/src/Tests/MigrateUpgradeTestBase.php. It passed this time so I'm changing this back to RTBC.

Thanks for the help @dcam and @afemath.

+++ b/core/modules/image/src/Tests/ImageFieldTestBase.php
@@ -126,6 +126,24 @@ function previewNodeImage($image, $field_name, $type) {
+   * @return array
+   *  A list containing the details of the generated image style.

If we're introducing a new helper to create an ImageStyle object I think we should behave like the other test entity creators and return the created entity. ie. this should just return the $style object and then the tests can call ->id(), ->label() and ->url(). Doing list() on the return looks unnecessarily magic.

Re rolled #28. Fixed #30.

currently i am working on it ......

currently checking the issue ......

Image effect help text is displaying when an effect is edited for drupal version 8.6.x-dev. Hence it is fixed. Please find the attached screenshot ....

Let Creator of this issue to mark "closed status".

Re-roll patch given for 8.9.x.

Fixed the tests.

Fixed fail test.

Certainly this will not get in 8.9. Bumping to 9.3.

Re-roll patch given for 9.3.x.

Fixed the CSpell issue.

1. --- /dev/null
   +++ b/core/modules/image/src/Tests/ImageAdminUiTest.php
   
   +++ b/core/modules/image/src/Tests/ImageAdminUiTest.php
   @@ -0,0 +1,62 @@
   
   @@ -0,0 +1,62 @@
   +<?php
   +
   +namespace Drupal\image\Tests;
   

   Oh... a Simpletest test! This is not executed any more, actually. To be converted to a Functional test.

2. +++ b/core/modules/image/src/Tests/ImageAdminUiTest.php
   @@ -0,0 +1,62 @@
   +  public function testAddEffectHelpText() {
   

   :void return typehint

3. +++ b/core/modules/image/src/Tests/ImageAdminUiTest.php
   @@ -0,0 +1,62 @@
   +    $this->assertText(t('Resizing will make images an exact set of dimensions. This may cause images to be stretched or shrunk disproportionately.'), 'The image style effect help text was displayed on the add effect page.');
   

   assertText() is deprecated

4. +++ b/core/modules/image/src/Tests/ImageAdminUiTest.php
   @@ -0,0 +1,62 @@
   +  public function testEditEffectHelpText() {
   

   :void return typehint

5. +++ b/core/modules/image/src/Tests/ImageAdminUiTest.php
   @@ -0,0 +1,62 @@
   +    $this->drupalPostForm($style_path . '/add/image_resize', $edit, t('Add effect'));
   

   drupalPostForm() is deprecated

6. +++ b/core/modules/image/src/Tests/ImageAdminUiTest.php
   @@ -0,0 +1,62 @@
   +      $this->assertText(t('Resizing will make images an exact set of dimensions. This may cause images to be stretched or shrunk disproportionately.'), 'The image style effect help text was displayed on the add effect page.');
   

   assertText() is deprecated

7. +++ b/core/modules/image/tests/src/Functional/ImageFieldTestBase.php
   @@ -89,6 +89,25 @@ public function previewNodeImage($image, $field_name, $type) {
   +  public function createRandomStyle() {
   

   Needs a return typehint

8. +++ b/core/modules/image/tests/src/Functional/ImageFieldTestBase.php
   @@ -89,6 +89,25 @@ public function previewNodeImage($image, $field_name, $type) {
   +    return [
   +      $style,
   +      $style_name,
   +      'admin/config/media/image-styles/manage/' . $style_name,
   +    ];
   +  }
   

   Do we really need to return an array? Maybe this would do in D7, but now we should be able to retrieve the style's name and Url from the object itself?

Worked on #65:
#65.1: Converted to functional test.
#65.2: :void typehint added.
#65.3: Fixed.
#65.4: :void typehint added.
#65.5: Fixed.
#65.6: Fixed.
#65.7: :array typehint added.
#65.8: Pending.

1. +++ b/core/modules/image/tests/src/Functional/ImageAdminUiTest.php
   @@ -0,0 +1,68 @@
   +    $this->assertSession()->pageTextContains(t('Resizing will make images an exact set of dimensions. This may cause images to be stretched or shrunk disproportionately.'));
   ...
   +      $this->assertSession()->pageTextContains(t('Resizing will make images an exact set of dimensions. This may cause images to be stretched or shrunk disproportionately.'));
   

   There's no need to wrap the string in t() here, we are not testing translations.

2. +++ b/core/modules/image/tests/src/Functional/ImageFieldTestBase.php
   @@ -89,6 +89,25 @@ public function previewNodeImage($image, $field_name, $type) {
   +    return [
   +      $style,
   +      $style_name,
   +      'admin/config/media/image-styles/manage/' . $style_name,
   +    ];
   

   just return $style. Then in calling code, the style name would be $style->getName(), and the path $style->toUrl() or even $style->toUrl()->toString().

Let me work on this

This is test only code, so removing irrelevant tag

Worked on #67, Please have a look.

Updated the title a bit, to be more in line with the patch.

1. +++ b/core/modules/image/tests/src/Functional/ImageAdminUiTest.php
   @@ -0,0 +1,68 @@
   +  protected $defaultTheme = 'classy';
   

   This should now be 'stark' I guess

2. +++ b/core/modules/image/tests/src/Functional/ImageAdminUiTest.php
   @@ -0,0 +1,68 @@
   +    // Open the edit effect form and check image style effect help text.
   +    $style = ImageStyle::load($style->getName());
   +    $effects = $style->get('effects');
   +    foreach ($effects as $ieid => $effect) {
   +      $this->drupalGet($style->toUrl()->toString() . '/effects/' . $ieid);
   ...
   +      $this->assertSession()->pageTextContains('Resizing will make images an exact set of dimensions. This may cause images to be stretched or shrunk disproportionately.');
   

   We should check that $effects isn't empty (or size 1, since we added one effect earlier), because if it is empty, this will still pass

Please review.

Reviewing MR 3270

Removing credit for myself as I just rebased to make sure the tests passed.

The changes requested in #74 have been addressed
All new functions have a return typehint.

Thanks for working on this issue! A few points:

1. +++ b/core/modules/image/tests/src/Functional/ImageAdminStylesTest.php
   @@ -316,10 +316,7 @@ public function testStyle() {
   -    $style_name = strtolower($this->randomMachineName(10));
   -    $style_label = $this->randomString();
   -    $style = ImageStyle::create(['name' => $style_name, 'label' => $style_label]);
   -    $style->save();
   +    $style = $this->createRandomStyle();
        $style_path = 'admin/config/media/image-styles/manage/';
   
   +++ b/core/modules/image/tests/src/Functional/ImageFieldTestBase.php
   @@ -89,6 +89,21 @@ public function previewNodeImage($image, $field_name, $type) {
   +  public function createRandomStyle(): object {
   +    $style_name = strtolower($this->randomMachineName());
   +    $style_label = $this->randomString();
   +    $values = ['name' => $style_name, 'label' => $style_label];
   +    $style = \Drupal::entityTypeManager()->getStorage('image_style')->create($values);
   +    $style->save();
   +    return $style;
   

   In general, Drupal has moved away from creating random fixture data, since it can cause all sorts of random failures and obscure what is actually being tested. We're better off having a fixed value (or data provider of values covering edgecases). I can see the value of having a helper method to save a new image style, but I think it should probably take the style name and label as parameters rather than being random.

   Also, we should almost never typehint a generic object. Surely there's an interface for image styles?

2. +++ b/core/modules/image/tests/src/Functional/ImageAdminUiTest.php
   @@ -0,0 +1,68 @@
   + * Tests the administrative user interface.
   

   That would be a really big test...

   I think this is supposed to be "Tests the image style administration UI" or something along those lines.

3. +++ b/core/modules/image/tests/src/Functional/ImageAdminUiTest.php
   @@ -0,0 +1,68 @@
   +   * Test if the help text is available on the edit effect form.
   ...
   +   * Test if the help text is available on the edit effect form.
   

   Nit: "Tests that..."

   Also, are these two docblocks supposed to be identical? Seems like they should be different.

4. +++ b/core/modules/image/tests/src/Functional/ImageAdminUiTest.php
   @@ -0,0 +1,68 @@
   +    $this->submitForm($edit, t('Add effect'));
   

   I don't think we need the use of t() here.

I just realized this issue has both patches and an MR, so I was reviewing the wrong thing. Most of my feedback still applies, though.

Hiding the patches, but please apply the above feedback to the MR. Thanks!

I made the xjm's suggestions, please review.

Points from #81 have been addressed.

See the comments in the MR.

It has been two years since @dhirendra.mishra worked on this, so un-asssinging.

I made the @quietone's suggestions.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Still appears to have one open thread.

Seems MR has some kind of failure.

Moving to Needs Review.

I think this could probably be a kernel test no? Does it need a full bootstrap for testing a message? If it remains a functional still don't think it needs to be it's own.

Made some minor updates and left a few comments. I think this is fine as a functional test as the issue is about help text appearing on specific routes.

Keeping as NW because I feel like there are some changes that are out of scope here.