Problem/Motivation

Because of #3083275: [meta] Update tests that rely on Classy to not rely on it anymore and Classy being deprecated in Drupal 9 + removed in Drupal 10,: Tests that aren't specifically testing Classy yet declare $defaultTheme = 'classy'; should be refactored to use Stark as the default theme instead.

Proposed resolution

Change all tests in this module to use Stark as the default theme, and refactor the tests where needed so they continue to function properly.

Please note that because of #3271057: Move Media Library CKEditor 4 integrations from Media into CKEditor, we will not modify Drupal\Tests\media_library\FunctionalJavascript\CKEditorIntegrationTest in this issue.

CommentFileSizeAuthor
#11 interdiff_8-11.txt1.04 KBdanflanagan8
#11 3278418-11.patch18.9 KBdanflanagan8
#8 3278418-8.patch18.84 KBdanflanagan8
#7 3278418-7.patch18.84 KBdanflanagan8

Comments

danflanagan8 created an issue. See original summary.

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
Issue summary: View changes
Related issues: +#3271057: Move Media Library CKEditor 4 integrations from Media into CKEditor
danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented

There are numerous test classes that extend MediaLibraryTestBase but do not declare their own defaultTheme. It is important that we careful check those test classes as well. Here are some notes on those.

  1. WidgetWithoutTypesTest (no change required)
  2. FieldUiIntegrationTest (no change required)
  3. WidgetUploadTest (no changes required)
  4. WidgetAccessTest (requires different selector for view)
  5. ViewsUiIntegrationTest (requires removal of class from selector, which was already over-selected)
  6. MediaOverviewTest (requires change to view selector and a change to a negative assertion so it is still meaningful with stark)
  7. EntityReferenceWidgetTest (needs lots of work)
  8. WidgetOEmbedTest (have to somehow replace
    $assert_session->elementNotExists('css', '.media-library-add-form__selected-media');</li>
  <li>

    )

  9. WidgetViewsTest (needs updated assertions for pager since is-activeclass comes from classy)

Also, I'm not quite sure what to do with this assertion in MediaLibraryTestBase::assertMediaAdded:

$assert_session->elementNotExists('css', '.file-size', $fields);

The file-size class gets added by stable/bartik/seven.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
Related issues: +#3117430: file-link template should not always display file_size

Regarding:

Also, I'm not quite sure what to do with this assertion in MediaLibraryTestBase::assertMediaAdded:

$assert_session->elementNotExists('css', '.file-size', $fields);

The file-size class gets added by stable/bartik/seven.

I've dug a bit deeper. The comment a few lines above that assertion reads:

// Assert extraneous components were removed in
// FileUploadForm::hideExtraSourceFieldComponents().

That method does not do anything directly related to the file-size class or the markup. The assertion really has no business being there. It's a true assertion in some cases, but media_library doesn't have anything to do with it really. It's also essentially duplicating the assertion above it (which is important and meaningful) that reads:

$assert_session->elementNotExists('css', '[data-drupal-selector$="filename"]', $fields);

That form element whose existence is tested uses the file_link theme, which is where the file-size class is added by some themes. But of course the file-size class won't be there if the element using the 

file_link

theme is not there. Right?

Long story short, I'm going to remove the assertion.

There are zero other assertions in the codebase that relate to file-size, but if any assertions are needed, they should be added as part of #3117430: file-link template should not always display file_size.

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented

For my personal benefit, I have addressed everything in #3 other than item 7 and item 9.

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
Assigned: danflanagan8 » Unassigned
Status: Active » Needs review
StatusFileSize
new 3278418-7.patch18.84 KB

For EntityReferenceWidgetTest I decided it needs classy so I made a followup to switch it to starterkit. Here's the issue: #3281695: Remove Classy dependency of Media Library's EntityReferenceWidgetTest

Also, per the IS, we are not addressing Drupal\Tests\media_library\FunctionalJavascript\CKEditorIntegrationTest in this issue.

Additional notes for reviewers:

In addition to the obvious changes to css selectors required, I felt the need to improve several assertions in WidgetOEmbedTest and WidgetViewsTest.

I also felt compelled to remove an unnecessary assertion in MediaLibraryTestBase without feeling any compulsion to replace it. That's described in #5.

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
StatusFileSize
new 3278418-8.patch18.84 KB

Oops! Let's try this one.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Looks good!

quietone’s picture

quietone commented
Status: Reviewed & tested by the community » Needs work

Nice to see progress here.

The Issue Summary says that Drupal\Tests\media_library\FunctionalJavascript\CKEditorIntegrationTest is not addressed here. I think the issue where that is being taken care of should be included in the IS.

Can we have a more thorough review? I think the patch should be applied locally and then some searching done to ensure that all the tests have been changed. Has the code been reviewed?

I am not qualified to review the code on this but I read the comments and have some questions.

  1. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/EntityReferenceWidgetTest.php
@@ -19,6 +19,19 @@ class EntityReferenceWidgetTest extends MediaLibraryTestBase {
+   * @todo This test's reliance on classes in order to assert the order of
+   *   field items makes Stark a bad fit as a base theme. Change the default
+   *   theme to Starterkit once it is stable.

    I had to read this more that once. Maybe this version will save others the same problem

    '@todo Tests normally uses Stark but that is not suitable because this test relies on classes to assert the order of field items. Change this to Starterkit once it is stable.'

    Just a suggestion. 

  2. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaOverviewTest.php
@@ -78,8 +83,11 @@ public function testAdministrationPage() {
+    // Verify that the media name does not contain a link. The selector is
+    // tricky, so start by assuring ourselves the selector is good.

    What does 'the selector is good' mean? Does it mean 'valid and exists'? But then what is a 'bad' selector?

Back to NW!

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
Status: Needs work » Needs review
StatusFileSize
new 3278418-11.patch18.9 KB
new interdiff_8-11.txt1.04 KB

Thanks, @quietone!

10.1: I'm using @xjm's proposed template from the parent issue so I'm going to stick with that: https://www.drupal.org/project/drupal/issues/3083275#comment-14434087

10.2: A fair critique! This updated patch tries to clarify.

On the IS, the link to the issue is already there. And it looks like it's fixed as of about a month ago, which is nice.

Can we have a more thorough review?

I'm going to throw this back to NR to see if we can't get someone to do a more thorough review. Perhaps the reviewer could comment on my comments #5 and/or #7 as part of the review. Those are more substantive changes than are typically happening in these Classy-to-Stark issues.

Thanks!

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs review » Reviewed & tested by the community

Except the remaining classy that has a comment pointing out why it's still here I can confirm we don't have classy in the various test methods of the media_library module.

The extra checks make sense to me, I don't see that hurting or slowing down things. most of the changes are swaping one class name for another, or use data-drupal-selector when possible instead of class name. As for the extra tests they make sense to me and test what they say they're testing.

  • lauriii committed f384111 on 10.1.x 
    Issue #3278418 by danflanagan8, quietone, nod_: Media Library Tests...

  • lauriii committed 64de78b on 10.0.x 
    Issue #3278418 by danflanagan8, quietone, nod_: Media Library Tests...

  • lauriii committed 34ab385 on 9.5.x 
    Issue #3278418 by danflanagan8, quietone, nod_: Media Library Tests...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Reviewed & tested by the community » Fixed

The text on #10.1 is something we've used on multiple tests so it seems like it should be fine here too.

Committed f384111 and pushed to 10.1.x. Also cherry-picked to 10.0.x and 9.5.x. Thanks!

Status: Fixed » Closed (fixed)

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