In AttachedAssetsTest.php, testAggregation(), there is a test that tests 3 related things in 1 test. This should be broken up into 3 separate tests to make it easier to figure out what is wrong when the test fails.

CommentFileSizeAuthor
#5 drupal-separate_tests-2601790-5-D8.patch1.52 KBliam morland
#2 drupal-separate_tests-2601790-2-D8.patch1.53 KBliam morland

Comments

Liam Morland created an issue. See original summary.

liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented
Status: Active » Needs review
StatusFileSize
new drupal-separate_tests-2601790-2-D8.patch1.53 KB
liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented
Parent issue: » #1494670: References to CSS, JS, and similar files should be root-relative URLs: avoids mixed content warnings & fewer bytes to send
dawehner’s picture

dawehner
Primary language German
commented
Issue tags: +rc eligible

<3

+++ b/core/modules/system/src/Tests/Common/AttachedAssetsTest.php
@@ -182,12 +182,9 @@ function testAggregation() {
+    $this->assertTrue(count($rendered_footer_js) == 2, 'There are 2 JavaScript assets in the footer.');
+    $this->assertTrue($rendered_footer_js[0]['#attributes']['data-drupal-selector'] === 'drupal-settings-json', 'The first of the two JavaScript assets in the footer has drupal settings.');
+    $this->assertTrue(substr($rendered_footer_js[1]['#attributes']['src'], 0, 7) === 'http://', 'The second of the two JavaScript assets in the footer has the sole aggregated JavaScript asset.');

IMHO We should use asertEqual instead

liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented
StatusFileSize
new drupal-separate_tests-2601790-5-D8.patch1.52 KB

Thanks. Patch using assertEqual().

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Great, thank you!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 5: drupal-separate_tests-2601790-5-D8.patch, failed testing.

liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented
Status: Needs work » Reviewed & tested by the community

Restoring status given in #6.

dawehner’s picture

dawehner
Primary language German
commented
Title: In AttachedAssetsTest.php, separate a 3-part test into 3 separate tests » In AttachedAssetsTest.php, separate a 3-part test into 3 separate test assertions

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 5: drupal-separate_tests-2601790-5-D8.patch, failed testing.

liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented
Status: Needs work » Reviewed & tested by the community

Restoring status given in #6.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 3bd42a2 and pushed to 8.0.x. Thanks!

  • alexpott committed 3bd42a2 on 8.0.x 
    Issue #2601790 by Liam Morland: In AttachedAssetsTest.php, separate a 3-...

Status: Fixed » Closed (fixed)

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