Problem/Motivation

StableDecoupledTest is now gone from 9.1.x branch.
However, it'll remain in the 9.0.x branch.
At #3138652: Remove StableDecoupledTest I raised some issues with it, based on my experience from #3033734: Horizontal scrollbar on /admin/modules:

A) $this->assertEmpty($files_inherited_from_stable); The default PHPUnit message when this assertion fails is just "Failed asserting that an array is empty." In #3033734 I had to add some debugging code to the test when running it locally to confirm system.admin.css was indeed the problem. It'd be really nice if the test output told us that directly.

B) If this test is going to fail (by design) whenever stable diverges from core markup, can we add some more comments to help folks understand how to properly fix the test in this (potentially common) case of failure? I.e. make it more obvious that 3033734.49_51.interdiff.txt is the intended solution whenever stable and core intentionally diverge?

C) There are a handful of typos in the comments.

Proposed resolution

Fix the things.

Remaining tasks

  1. Do it.
  2. Reviews.
  3. RTBC.
  4. Commit to 9.0.x.

User interface changes

N/A

API changes

N/A

Data model changes

N/A

Release notes snippet

N/A

CommentFileSizeAuthor
#2 3138739-2.patch3.4 KBdww
#2 3138739-2.test-only.patch5.7 KBdww

Comments

dww created an issue. See original summary.

dww’s picture

dww
we/he/they
commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new 3138739-2.test-only.patch5.7 KB
new 3138739-2.patch3.4 KB

This fixes A, B and C.

I'm also including a "test-only" version that introduces a CSS change to system.admin.css from #3033734-49: Horizontal scrollbar on /admin/modules and a drupalci.yml change to only run this test so you can see the improved test output from A.

Thoughts?

Thanks,
-Derek

The last submitted patch, 2: 3138739-2.test-only.patch, failed testing. View results

dww’s picture

dww
we/he/they
commented
Title: Improve StableDecoupledTest » Improve StableDecoupledTest in the 9.0.x branch

Yay, that test-only failure output looks much better. ;)

Here:

1) Drupal\KernelTests\Core\Theme\StableDecoupledTest::testDecoupledStable with data set "claro" ('core/themes/claro', array('block.admin.css', 'filter.caption.css', 'progress.module.css', 'status-report.html.twig', 'system-modules-details.html.twig', 'system-modules-uninstall.html.twig'))
There should be no assets inhereted from Stable, but the following files are inhereted: array (
  0 => 'system.admin.css',
)
Failed asserting that an array is empty.

vs. the original fail that prompted me to open this:

1) Drupal\KernelTests\Core\Theme\StableDecoupledTest::testDecoupledStable with data set "claro" ('core/themes/claro', array('block.admin.css', 'filter.caption.css', 'progress.module.css', 'status-report.html.twig', 'system-modules-details.html.twig', 'system-modules-uninstall.html.twig'))
Failed asserting that an array is empty.

I tried to do the fancy thing with FormattableMarkup and a placeholder, but the var_export() formatting always gets HTML escaping, which makes it harder to read:

1) Drupal\KernelTests\Core\Theme\StableDecoupledTest::testDecoupledStable with data set "claro" ('core/themes/claro', array('block.admin.css', 'filter.caption.css', 'progress.module.css', 'status-report.html.twig', 'system-modules-details.html.twig', 'system-modules-uninstall.html.twig'))
There should be no assets inhereted from Stable, but the following files are inhereted: array (
  0 => 'system.admin.css',
)
Failed asserting that an array is empty.

Given that this is auto-generated output based on core filenames, I can't fathom why we'd want/need any of this "properly escaped" -- it's just CLI output, not HTML in a browser...

Also, updating title to make it clear this is 9.0.x-only change.

Thanks,
-Derek

Status: Needs review » Needs work

The last submitted patch, 2: 3138739-2.patch, failed testing. View results

dww’s picture

dww
we/he/they
commented
Status: Needs work » Needs review

Grrr @ media_library tests...
Re-queued.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Status: Needs review » Needs work 
  1. +++ b/core/tests/Drupal/KernelTests/Core/Theme/StableDecoupledTest.php
@@ -174,14 +181,18 @@ public function testDecoupledStable($path, array $to_skip) {
+    $this->assertEmpty($files_inherited_from_stable, 'There should be no assets inhereted from Stable, but the following files are inhereted: ' . var_export(array_values($files_inherited_from_stable), TRUE));

    s/inhereted/inherited

    After core themes were decoupled from Stable, the variable name $files_inherited_from_stable became inaccurate, so this added error message is accurate either. Now that decoupling has happened, nothing is inherited from Stable. A more accurate name at this point might be $files_that_are_expected_to_match_stable. The error message could be something like: "these files in Stable: (filenames) were expected to match their counterparts in core. If any of these files have intentionally deviated from core, add them to the to-skip array for each theme in providerTestDecoupledStable()". 

  2. +++ b/core/tests/Drupal/KernelTests/Core/Theme/StableDecoupledTest.php
@@ -174,14 +181,18 @@ public function testDecoupledStable($path, array $to_skip) {
    * The to-skip arrays should become increasingly smaller as issues that
-   * address Stable library dependencies are completed.
+   * address Stable library dependencies are completed. However, they will grow
+   * if bugs are fixed in core assets in the 9.0.x branch. In those cases, we
+   * have to leave Stable alone, but the difference between the stable and core
+   * asset will be flagged as a test failure and we'll need to add those files
+   * to the to-skip lists.

    The first line is also no longer accurate. The "increasingly smaller" already happened in Q1 2020, so this whole comment would need a rewrite to be accurate.

    The to-skip array is for assets that are
    1) Previously inherited from Stable and now inherited from core. (i.e. not overridden)
    2) They are in some way different from their equivalents in Stable, but those differences are confirmed to be acceptable within the theme being tested.

It's apparent after this review that changes to this test (and possibly even removing it) should have taken place as part of #3115223: Remove Stable as a base theme of core themes, so mitigating some of the confusion from that omission is appreciated.

narendra.rajwar27’s picture

narendra.rajwar27
he/him
Primary language Hindi
Location India
commented
Assigned: Unassigned » narendra.rajwar27
narendra.rajwar27’s picture

narendra.rajwar27
he/him
Primary language Hindi
Location India
commented
Assigned: narendra.rajwar27 » Unassigned

Version: 9.0.x-dev » 9.1.x-dev

Drupal 9.0.10 was released on December 3, 2020 and is the final full bugfix release for the Drupal 9.0.x series. Drupal 9.0.x will not receive any further development aside from security fixes. Sites should update to Drupal 9.1.0 to continue receiving regular bugfixes.

Drupal-9-only bug reports should be targeted for the 9.1.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.2.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.1.x-dev » 9.3.x-dev

Drupal 9.1.10 (June 4, 2021) and Drupal 9.2.10 (November 24, 2021) were the last bugfix releases of those minor version series. Drupal 9 bug reports should be targeted for the 9.3.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

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

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should 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.

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

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

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

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

quietone commented
Status: Needs work » Closed (outdated)

@dww, thanks for making a patch to improve the test. However, Drupal 9 is EOL, therefor I am closing this one.

Cheers