Problem/Motivation

Adding the patch in #40 here: #2105583: Add some sane strictness to phpunit tests to catch risky tests

...we discover some tests that need attention.

$ ./vendor/bin/phpunit -c core --filter ComposerIntegrationTest -v

.R

Time: 13.47 seconds, Memory: 144.00Mb

There was 1 risky test:

1) Drupal\Tests\ComposerIntegrationTest::testAllModulesReplaced
This test did not perform any assertions

ComposerIntegrationTest::testAllModulesReplaced() makes a few assumptions that don't show up as fails, if you're not running in strict mode.

Mainly that $composerjson->replace is an array, when it's an object.

Proposed resolution

Fix the test.

Fixing the test leads to a fail, which we also fix here.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#9 interdiff.txt1.18 KBMile23
#9 2597644_9.patch2.18 KBMile23
#2 2597644_1.patch2.18 KBMile23
#2 2597644_1_test_only.patch1.76 KBMile23
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Mile23 created an issue. See original summary.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Status: Active » Needs review
FileSize
2597644_1_test_only.patch1.76 KB
2597644_1.patch2.18 KB

The test_only patch fixes the test, the other patch makes the test pass.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Issue summary: View changes
Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Issue summary: View changes

The last submitted patch, 2: 2597644_1_test_only.patch, failed testing.

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented
Status: Needs review » Reviewed & tested by the community

Nice! After wrapping my head around it I like the modified assertion.

+++ b/core/tests/Drupal/Tests/ComposerIntegrationTest.php
@@ -75,20 +75,33 @@ public function testComposerJson() {
+      if ((!in_array($file_name, $discard)) && is_dir($module_path . '/' . $file_name)) {

For future reviewers, this is the key change. Fixing the is_dir() so $module_names is correctly populated and the later assertion in the foreach gets run.

The last submitted patch, 2: 2597644_1_test_only.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented

Fixable on commit.

  1. +++ b/core/composer.json
@@ -88,6 +88,7 @@
+    "drupal/inline_form_errors": "self.version",

    Oops, guess I missed this part. 

  2. +++ b/core/tests/Drupal/Tests/ComposerIntegrationTest.php
@@ -75,20 +75,33 @@ public function testComposerJson() {
+    $composer_replace_packages = (array)$json->replace;

    missing space after the cast 

  3. +++ b/core/tests/Drupal/Tests/ComposerIntegrationTest.php
@@ -75,20 +75,33 @@ public function testComposerJson() {
+        'drupal/'.$module_name,

    Missing spaces around concatenation

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Status: Reviewed & tested by the community » Needs review
FileSize
2597644_9.patch2.18 KB
interdiff.txt1.18 KB

Actually there's a couple more things that are 'fixable on commit' but I'll get them here. :-)

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Status: Needs review » Reviewed & tested by the community

Resetting to RTBC, since it's just a comment change, and addresses the other coding standards issues. Sorry if that's presumptuous.

The last submitted patch, 2: 2597644_1_test_only.patch, failed testing.

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented

+1

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler at erdfisch commented

Looks good to me too, RTBC++

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler at erdfisch commented
Status: Reviewed & tested by the community » Needs review

Hmm... is there a reason ComposerIntegrationTest is not to be found on https://dispatcher.drupalci.org/job/default/25922/console ?

The last submitted patch, 2: 2597644_1_test_only.patch, failed testing.

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented
Status: Needs review » Reviewed & tested by the community

Maybe you didn't have it expanded to show the full results?
On that run:
00:24:01 Drupal\Tests\ComposerIntegrationTest 2 passes
On the last run:
11:01:58 Drupal\Tests\ComposerIntegrationTest 2 passes

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler at erdfisch commented

Ahh @neclimdul, thanks! I totally missed that. Yes, searching on https://dispatcher.drupalci.org/job/default/25922/consoleFull yields the expected result. Sorry!

The last submitted patch, 2: 2597644_1_test_only.patch, failed testing.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Issue tags: -rc target triage +rc eligible
alexpott’s picture

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

Committed 1e0d343 and pushed to 8.0.x. Thanks!

  • alexpott committed 15ccf7f on 8.0.x 
    Issue #2597644 by Mile23: ComposerIntegrationTest::...

Status: Fixed » Closed (fixed)

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