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

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..."

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.

sahil.goyal → made their first commit to this issue’s fork.

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.