Create Classy library dependency tests that can be used for all themes, and verify by providing an Umami-specific classy/dropbutton

+++ b/core/profiles/demo_umami/themes/umami/umami.info.yml
@@ -16,6 +16,8 @@ libraries-override:
+  core/drupal.dropbutton:
+    - umami/classy.dropbutton

Should we also add a library override to unload the library from Classy? If we don't unload the Classy library, we would have two versions of the CSS loaded until we have removed dependency to Classy.

1. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeNotUsingClassyLibraryTest.php
   @@ -0,0 +1,329 @@
   +      // If the library override has a css key, it's some classy CSS is still
   

   Nit: s/css/CSS

2. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeNotUsingClassyLibraryTest.php
   @@ -0,0 +1,329 @@
   +      if (in_array($classy_library_name, $libraries_to_skip) || isset($library_overrides_created_by_this_theme["classy/$classy_library_name"])) {
   ...
   +        $this->assertNotEmpty($theme_css_files);
   +        $this->assertEmpty(array_diff($theme_css_files, $classy_css_files), "Missing the inclusion of one or more CSS files from classy/$classy_library_name.");
   ...
   +        $this->assertNotEmpty($theme_js_files);
   +        $this->assertEmpty(array_diff($theme_js_files, $classy_js_files), "Missing the inclusion of one or more JS files from classy/$classy_library_name.");
   

   These assertions are never run given that we skip this if the library has been overridden.

3. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeNotUsingClassyLibraryTest.php
   @@ -0,0 +1,329 @@
   +      // Confirm there is a theme-specific version of the Classy library.
   +      $this->assertArrayHasKey("classy.$classy_library_name", $this_theme_libraries, "There is not a $theme equivalent for classy/$classy_library_name");
   

   This seems redundant given that we never run this assert if the library has been overridden 🤔

4. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeNotUsingClassyLibraryTest.php
   @@ -0,0 +1,329 @@
   +      // that should be extended instead of overridden.
   ...
   +      $this->assertTrue(($extends_core_library || $overrides_core_library), "$library_extended is extended by Classy and should be extended or overridden by $theme");
   ...
   +        // Libraries-extend to skip in test.
   

   Should we add more specific assertions for this scenario?

I tested manually various use cases to ensure that the tests fail when libraries are not sufficiently overridden. I tried missing override on a library that was extended, as well as libraries with both CSS and JavaScript dependencies. Looks good to me! 🚀

Thanks for all the excellent test patches exposing the specific fails!

Reviewed so far by:

• Copying core/themes/classy/css/components/dropbutton.css to core/profiles/demo_umami/themes/umami/css/classy/components/ with the patch applied to verify that the files are identical.
• Checking the diffs between the exposed fail patches in #2 and the full patches -- the relevant bits are:
  • Missing library extension:
    

    +++ b/core/profiles/demo_umami/themes/umami/umami.info.yml
    @@ -15,8 +15,8 @@ libraries-override:
     libraries-extend:
       tour/tour-styling:
         - umami/demo-umami-tour
    -  core/drupal.dropbutton:
    -    - umami/classy.dropbutton
    +#  core/drupal.dropbutton:
    +#    - umami/classy.dropbutton
     

    Failure message

    1) Drupal\KernelTests\Core\Theme\UmamiClassyLibraryTest::testUmamiCopiesOfClassyLibraries
    Umami should override or extend classy/dropbutton.
    Failed asserting that an array has the key 'classy/dropbutton'.
    

  • Missing equivalent file:
    

         component:
    -      css/classy/components/dropbutton.css: { weight: -10 }
    +      css/classy/components/dropbutton-different.css: { weight: -10 }
    diff --git a/core/tests/Drupal/KernelTests/Core/Theme/UmamiClassyLibraryTest.php b/core/tests/Drupal/KernelTests/Core/Theme/UmamiClassyLibraryTest.php
    index b68e0414cf..9699a7116c 100644
    --- a/core/tests/Drupal/KernelTests/Core/Theme/UmamiClassyLibraryTest.php
    +++ b/core/tests/Drupal/KernelTests/Core/Theme/UmamiClassyLibraryTest.php
    @@ -175,7 +175,7 @@ public function testUmamiCopiesOfClassyLibraries() {
             continue;
           }
     
    -      $this->assertArrayHasKey("classy/$classy_library_name", $umami_library_overrides, "Umami should override or extend classy/$classy_library_name.");
    +      $this->assertArrayHasKey("classy/$classy_library_name", $umami_library_overrides, "Umami should override classy/$classy_library_name.");
           $this->assertEmpty($umami_library_overrides["classy/$classy_library_name"], "Umami's override for classy/$classy_library_name should be empty.");
         }
    

    Failure message:

    1) Drupal\KernelTests\Core\Theme\UmamiClassyLibraryTest::testUmamiCopiesOfClassyLibraries
    Missing the inclusion of one or more CSS files from classy/dropbutton.
    Failed asserting that an array is empty.

  • File not present:
    

    rename from core/profiles/demo_umami/themes/umami/css/classy/components/dropbutton.css
    rename to core/profiles/demo_umami/themes/umami/css/classy/components/dropbutton-different.css
    diff --git a/core/tests/Drupal/KernelTests/Core/Theme/UmamiClassyLibraryTest.php b/core/tests/Drupal/KernelTests/Core/Theme/UmamiClassyLibraryTest.php
    index b68e0414cf..9699a7116c 100644
    --- a/core/tests/Drupal/KernelTests/Core/Theme/UmamiClassyLibraryTest.php
    +++ b/core/tests/Drupal/KernelTests/Core/Theme/UmamiClassyLibraryTest.php
    @@ -175,7 +175,7 @@ public function testUmamiCopiesOfClassyLibraries() {
             continue;
           }
     
    -      $this->assertArrayHasKey("classy/$classy_library_name", $umami_library_overrides, "Umami should override or extend classy/$classy_library_name.");
    +      $this->assertArrayHasKey("classy/$classy_library_name", $umami_library_overrides, "Umami should override classy/$classy_library_name.");
           $this->assertEmpty($umami_library_overrides["classy/$classy_library_name"], "Umami's override for classy/$classy_library_name should be empty.");
         }
    

    Failure message:

    1) Drupal\KernelTests\Core\Theme\UmamiClassyLibraryTest::testUmamiCopiesOfClassyLibraries
    File core/profiles/demo_umami/themes/umami/css/classy/components/dropbutton.css does not exist.
    Failed asserting that file "/var/www/html/core/profiles/demo_umami/themes/umami/css/classy/components/dropbutton.css" exists.
    

  • No replacement library:

    +#classy.dropbutton:
    +#  version: VERSION
    +#  css:
    +#    component:
    +#      css/classy/components/dropbutton.css: { weight: -10 }
    diff --git a/core/tests/Drupal/KernelTests/Core/Theme/UmamiClassyLibraryTest.php b/core/tests/Drupal/KernelTests/Core/Theme/UmamiClassyLibraryTest.php
    index b68e0414cf..9699a7116c 100644
    --- a/core/tests/Drupal/KernelTests/Core/Theme/UmamiClassyLibraryTest.php
    +++ b/core/tests/Drupal/KernelTests/Core/Theme/UmamiClassyLibraryTest.php
    @@ -175,7 +175,7 @@ public function testUmamiCopiesOfClassyLibraries() {
             continue;
           }
     
    -      $this->assertArrayHasKey("classy/$classy_library_name", $umami_library_overrides, "Umami should override or extend classy/$classy_library_name.");
    +      $this->assertArrayHasKey("classy/$classy_library_name", $umami_library_overrides, "Umami should override classy/$classy_library_name.");

    Failure message:

    1) Drupal\KernelTests\Core\Theme\UmamiClassyLibraryTest::testUmamiCopiesOfClassyLibraries
    There is not an Umami equivalent for classy/dropbutton
    Failed asserting that an array has the key 'classy.dropbutton'.
    

Next doing a line-by-line CR of the patch...

Started a code review. Overall this test is excellent and thorough. I got kind of lost in the reviewing the method so I think there's some potential to simplify it with refactoring. Haven't finished reviewing yet but here are my notes so far (didn't want to lose them):

1. +++ b/core/profiles/demo_umami/themes/umami/umami.libraries.yml
   @@ -130,3 +130,9 @@ oneplusfourgrid_section:
   +      css/classy/components/dropbutton.css: { weight: -10 }
   

   Does the theme decoupling depend on this negative weight for the overridden library? If so, this would be important to document in both the decoupling instructions and the CR.

2. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeNotUsingClassyLibraryTest.php
   @@ -0,0 +1,382 @@
   +  public function testThemeNotUsingClassyLibraries($theme, $libraries_to_skip, $extends_to_skip) {
   

   General obeservation: This test method is really long. Can we factor it into smaller helper methods or such?

   Also, should/could some of the initializations we use over and over that don't change be moved to setUp()?

3. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeNotUsingClassyLibraryTest.php
   @@ -0,0 +1,382 @@
   +    $libraries_already_fully_overridden = [];
   

   Is the idea that we will update this variable as we complete the decoupling of each core library? So it'll eventually be a fairly long array?

   Does it mean libraries fully overridden for just one theme, or all core themes?

   I think we should add an inline comment explaining this array.

4. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeNotUsingClassyLibraryTest.php
   @@ -0,0 +1,382 @@
   +    $library_overrides = $this->themeInitialization->getActiveThemeByName($theme)->getLibrariesOverride();
   +    $library_overrides_created_by_this_theme = isset($library_overrides[$theme_path]) ? $library_overrides[$theme_path] : [];
   

   So this is a bit confusing. (Needs followup for the documentation of getLibrariesOverride, which doesn't explain the return structure at all.) If these are already the library overrides for this theme, as per the previous line, why are we checking the top-level key of the array for the path of the theme?

   This could maybe use an inline comment.

   Also, $library_overrides isn't used again (except when it's re-initialized later in the method; see later point). Should we avoid the extra variable definition?

5. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeNotUsingClassyLibraryTest.php
   @@ -0,0 +1,382 @@
   +    $this_theme_libraries = $this->libraryDiscovery->getLibrariesByExtension($theme);
   

   Nit about the variable naming: Doesn't $theme_ at the start of the variable name mean the theme name passed in at the start of the method? Do we need a $this_theme_ here? Otherwise it makes it seem like it's something different. Maybe we could use $this_theme_ everywhere?

6. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeNotUsingClassyLibraryTest.php
   @@ -0,0 +1,382 @@
   +      if (in_array(str_replace('classy/', '', $library_name), $libraries_to_skip) || substr($library_name, 0, 7) !== 'classy/') {
   +        continue;
   +      }
   

   I had to read this time several times to figure out where it was going. So this line is going over the list of library overrides, and checking for whether they're either on the list of libraries to skip (which is passed into the method from the data provider), or whether it already has classy in its library name (which means... that it's already in the Classy subdirectory?... and then only proceeding if neither of those conditions are true.

   Could use an inline comment.

7. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeNotUsingClassyLibraryTest.php
   @@ -0,0 +1,382 @@
   +    foreach ($classy_libraries as $classy_library_name => $classy_library) {
   +      if (in_array($classy_library_name, $libraries_to_skip) || in_array("classy/$classy_library_name", $libraries_already_fully_overridden)) {
   +        continue;
   +      }
   ...
   +      // This is looking specifically for overrides, so skip if it's a library
   +      // that should be extended instead of overridden.
   +      if (in_array($classy_library_name, $libraries_to_skip) || in_array($classy_library_name, array_values($this_themes_library_extendees))) {
   +        continue;
   +      }
   

   Didn't we already foreach this above with a different compound continue? I wonder if there's a way to simplify the method structure.

8. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeNotUsingClassyLibraryTest.php
   @@ -0,0 +1,382 @@
   +    $library_overrides = $this->themeInitialization->getActiveThemeByName($theme)->getLibrariesOverride();
   +    $library_overrides_created_by_this_theme = isset($library_overrides[$theme_path]) ? $library_overrides[$theme_path] : [];
   

   Aren't these two lines already at the start of the method? Why are we overwriting them here?

   If they're overwritten but the same, we can delete these two lines and move the inline comment above them to the top of the method. If they're being overwritten and changing, that's a bit brittle and we should change it.

9. Edit: Duplicate bullet.

Edit: Duplicate post.

@bnjmnm is working on a new patch, so updating issue status to reflect that.

+1 for the refactor; much easier to read.

A few nits and inane observations:

1. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeNotUsingClassyLibraryTest.php
   @@ -0,0 +1,486 @@
   +    // In some cases an overridden Classy library does not use any copied assets
   +    // from Classy. This array collects those so this test knows to skip
   +    // assertions specific to those copied assets.
   

   Still confused here... what are "some cases"?

2. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeNotUsingClassyLibraryTest.php
   @@ -0,0 +1,486 @@
   +    $theme_library_overrides = $this->themeInitialization->getActiveThemeByName($theme)->getLibrariesOverride()[$theme_path] ?? [];
   

   Null coalescing operator, fancy!

3. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeNotUsingClassyLibraryTest.php
   @@ -0,0 +1,486 @@
   +      // If $library assets is false or a string, the override is preventing the
   

   Missing underscore for $library_assets.

4. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeNotUsingClassyLibraryTest.php
   @@ -0,0 +1,486 @@
   +      // If the library override has a `css` key, some Classy CSS files may
   

   Nit: I think we should use single quotes rather than backticks around 'css', because the array key is an actual string.

Thanks 💯for the clear fail patches and explanations; definitely a big help for reviewing the issue.

Some docblock nits:

1. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeNotUsingClassyLibraryTest.php
   @@ -0,0 +1,486 @@
   +   * Classy's libraries.
   ...
   +   * Libraries that Classy extends.
   

   Now that we have these, I think extending the docblocks here a little would help make the whole test easier to grok. Edit: Plus an @see to the API method returning the arrays (if such exists) is nice.

2. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeNotUsingClassyLibraryTest.php
   @@ -0,0 +1,486 @@
   +   * @var array
   +   */
   +  protected $classyLibraries;
   ...
   +   * @var array
   +   */
   +  protected $classyLibrariesExtend;
   ...
   +   * @param array $library_assets
   +   *   The library assets.
   ...
   +   * @param array $classy_library_data
   +   *   The Classy library's data.
   

   Can we use a more specific data type than array, e.g. array[], string[][], etc.?

3. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeNotUsingClassyLibraryTest.php
   @@ -0,0 +1,486 @@
   +  /**
   +   * Ensures that a theme is decoupled from Classy libraries.
   +   *
   +   * @dataProvider providerTestThemeNotUsingClassyLibraries
   +   */
   +  public function testThemeNotUsingClassyLibraries($theme, array $libraries_to_skip) {
   ...
   +  /**
   +   * Checks for theme-specific equivalents of all Classy library-extends.
   +   *
   +   * @dataProvider providerTestThemeAccountsForClassyExtensions
   +   */
   +  public function testThemeAccountsForClassyExtensions($theme, array $extends_to_skip) {
   

   When test methods have a data provider, I like adding parameter documentation since that documents the data in the data provider nicely.

4. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeNotUsingClassyLibraryTest.php
   @@ -0,0 +1,486 @@
   +   *   The theme's copy of the classy library.
   

   Ubernit: "Classy" with a capital C.

5. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeNotUsingClassyLibraryTest.php
   @@ -0,0 +1,486 @@
   +   * @param string $theme_path
   +   *   The path to the active theme.
   

   It's the path to the theme being tested, correct?

1. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeNotUsingClassyLibraryTest.php
   @@ -0,0 +1,486 @@
   +    // Get a list of all css or js files loaded by the Classy library.
   

   Ubernit: "CSS and "JS" capitalized, since they're words in a sentence rather than a string literal here.

2. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeNotUsingClassyLibraryTest.php
   @@ -0,0 +1,486 @@
   +    $this->assertArrayHasKey($type, $theme_copy_of_classy_library);
   ...
   +    $this->assertNotEmpty($theme_assets);
   +    $this->assertNotEmpty($classy_assets);
   

   💯 for starting off assertions like these to avoid false positives or weird fails later. Nice and robust.

3. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeNotUsingClassyLibraryTest.php
   @@ -0,0 +1,486 @@
   +      $key = str_replace("$theme_path/$type/classy/", '', $item['data']);
   +      $theme_assets[$key] = $item;
   

   So based on this $item['data'] is clearly a string, if that helps with the answer to my array type docs questions.

4. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeNotUsingClassyLibraryTest.php
   @@ -0,0 +1,486 @@
   +    $this->assertEmpty(array_diff_key($theme_assets, $classy_assets), "Missing the inclusion of one or more CSS files from classy/$classy_library_name.");
   

   Oops... in the assertion message, isn't it either CSS or JS? Or am I confused?

1. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeNotUsingClassyLibraryTest.php
   @@ -0,0 +1,486 @@
   +    // Get a list of libraries overridden by the current theme.
   ...
   +    // Get a list of libraries extended by the current theme.
   

   I guess part of where my theme-noob questions come into play is that I'm unfamiliar with what the difference is for a library to be "overridden" versus "extended". Is this terminology documented somewhere? I think that explanation in context will make everything else in the test easier to follow. A simple example in each case might help.

2. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeNotUsingClassyLibraryTest.php
   @@ -0,0 +1,486 @@
   +    // in the active theme by either 1) The active theme extends the core
   

   "active theme" again, which means something site-config-specific to me. In the comments above, we're saying "current theme". We're talking about the same thing, right? The theme as named in $theme, which is the theme currently being tested?

3. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeNotUsingClassyLibraryTest.php
   @@ -0,0 +1,486 @@
   +        $override_replaces_library = (gettype($overridden_with) === 'string');
   

   Hrm? I'm guessing this means that the item will contain string data as opposed to FALSE or NULL? Could use explanation.

4. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeNotUsingClassyLibraryTest.php
   @@ -0,0 +1,486 @@
   +          if (isset($overridden_with['js'])) {
   +            foreach ($overridden_with['js'] as $js_file => $options) {
   +              $this->assertFalse(strpos($js_file, 'core/themes/classy/js'), "Override is loading a classy asset: $css_file");
   +            }
   +          }
   

   Bug in the assertion message I think; shouldn't it be $js_file?

That's probably enough small feedback to merit a NW. Now I've read all the lines of the latest patch. :)

Thank you @bnjmnm for adding all the documenation! It makes understanding this test much easier 👏

1. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeNotUsingClassyLibraryTest.php
   @@ -0,0 +1,529 @@
   + * Tests that themes do not depend on Classy libraries.
   

   Should we document that this is specifically added to facilitate the process of decoupling from Classy and that this may be removed later?

2. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeNotUsingClassyLibraryTest.php
   @@ -0,0 +1,529 @@
   +      if ($in_skip_list || $not_classy_library || $library_replaced_by_another_library) {
   +        if ($library_replaced_by_another_library) {
   ...
   +          // Classy libraries. The should not be added to the
   

   Why are we checking this again? Isn't the previous check already enough?

3. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeNotUsingClassyLibraryTest.php
   @@ -0,0 +1,529 @@
   +          // Libraries with names that begin with `$theme/classy.` are copies of
   +          // Classy libraries. The should not be added to the
   +          // $libraries_without_copied_classy_assets array before exiting this
   +          // iteration of the loop.
   

   This comment is a bit confusing. It would be also useful if this would include explanation on to why we are not adding these libraries to the array.

4. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeNotUsingClassyLibraryTest.php
   @@ -0,0 +1,529 @@
   +                $this->assertFalse(strpos($css_file, 'core/themes/classy/css'), "Override is loading a classy asset: $css_file");
   ...
   +              $this->assertFalse(strpos($js_file, 'core/themes/classy/js'), "Override is loading a classy asset: $js_file");
   

   Nit pick: Classy with capital. 🔬👁

+++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeNotUsingClassyLibraryTest.php
@@ -7,6 +7,13 @@
+ * This test will fail when Classy is removed from Drupal core. At this point,
+ * this test will no longer be necessary and can be removed.

I'm not sure if we actually want hold removing this test until Classy is removed. It might make sense to allow themes to make changes to these library configuration before that. This could be discussed in a follow-up issue so maybe we just omit this piece of information now. I think this can be done on commit.

Besides that, this looks great ✨

1. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeNotUsingClassyLibraryTest.php
   @@ -0,0 +1,538 @@
   +   * Classy extends several core libraries with it's own assets, these are
   

   It should be "its" rather than "it's" in this sentence. ("It's" == "it is" but not "belonging to it".)

   Also, comma splice. I'd put a period after "assets" and then just capitalized "these" for a new sentence.

2. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeNotUsingClassyLibraryTest.php
   @@ -0,0 +1,538 @@
   +    // accounted for in the current theme by either 1) The current theme extends
   +    // the core library with local copy of the Classy library 2) Overriding the
   +    // core library altogether.
   

   For parallelism and agreement with "by", the first item should be "The current theme extending the core library..." (rather than "extends").

3. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeNotUsingClassyLibraryTest.php
   @@ -0,0 +1,538 @@
   +   * @param array $classy_library_data
   +   *   The Classy library's data.
   +   * @param array $theme_copy_of_classy_library
   +   *   The theme's copy of the Classy library.
   

   Same question about the specific array structure. We can at least say array[]. (Or maybe mixed[][], but the latter only occurs three times in core, so we can probably stick with array[].)

4. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeNotUsingClassyLibraryTest.php
   @@ -0,0 +1,538 @@
   +   * @return array
   +   *   Themes and the libraries to be ignored.
   ...
   +   * @return array
   +   *   Themes and the extensions to be ignored.
   

   This could also be array[] based on the values.

These fixes are small enough that uploading a new patch at RTBC would be fine.

Saving issue credit.

Committed to 9.0.x, and backported to 8.9.x also so that the diff between the branches is meaningful for the test. Thanks!