Problem/Motivation

There are several issues producing false positives in FeaturesManagerTest::testAssignInterPackageDependenciesWithoutBundle() and FeaturesManagerTest::testAssignInterPackageDependenciesWithBundle().

The main issues are:

  • $expected = $packages;

    $expected is a shallow copy of the array of packages, so we're in practice testing packages against themselves.

  • Several calls in FeaturesManagerTest::testAssignInterPackageDependenciesWithBundle() don't use forms appropriate for a bundle. Examples:

        $bundle->getFullName('package')->willReturn('package');

    Return value should be 'giraffe_package'.

        $bundle->isDefault()->willReturn(TRUE);

    Return value should be FALSE.

        $expected['package']->setDependencies(['my_other_feature', 'package2']);

    'package2' should be 'giraffe_package2'.

When fixed, the tests reveal two bugs:

  • We're calling FeaturesBundleInterface::getFullName() on an already prefixed package machine name.
  • We're mistakenly making the setting of an existing module dependency based on a config dependency contingent on there being a package assigned.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#7 interdiff.txt3.08 KBnedjo
#7 2657274-7.patch5.58 KBnedjo
#5 interdiff.txt4.27 KBnedjo
#5 2657274-3.patch4.13 KBnedjo
#2 2657274-2.patch1.04 KBnedjo
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nedjo created an issue. See original summary.

nedjo’s picture

nedjo
he/him/his
Primary language English
CreditAttribution: nedjo commented
Issue summary: View changes
FileSize
2657274-2.patch1.04 KB

Initial patch is limited to cloning the array of features bundles. This fails, suggesting problems either in our code or in our existing tests.

nedjo’s picture

nedjo
he/him/his
Primary language English
CreditAttribution: nedjo commented
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 2: 2657274-2.patch, failed testing.

nedjo’s picture

nedjo
he/him/his
Primary language English
CreditAttribution: nedjo commented
Status: Needs work » Needs review
FileSize
2657274-3.patch4.13 KB
interdiff.txt4.27 KB

Further fixing up of tests. Also, fix an error recently introduced to FeaturesManager::assignInterPackageDependencies(), where FeaturesBundleInterface::getFullName() is called on an already-prefixed package machine name.

Test is still failing:

There were 2 failures:

1) Drupal\Tests\features\Unit\FeaturesManagerTest::testAssignInterPackageDependenciesWithoutBundle
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    0 => 'my_other_feature'
-    1 => 'package2'
+    0 => 'package2'
 )

/path/to/site/modules/features/tests/src/Unit/FeaturesManagerTest.php:218

2) Drupal\Tests\features\Unit\FeaturesManagerTest::testAssignInterPackageDependenciesWithBundle
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
     0 => 'giraffe_package2'
-    1 => 'my_other_feature'
 )
/path/to/site/modules/features/tests/src/Unit/FeaturesManagerTest.php:257

An existing feature providing configuration required by an item should be added as a feature dependency, but apparently is not being added.

Status: Needs review » Needs work

The last submitted patch, 5: 2657274-3.patch, failed testing.

nedjo’s picture

nedjo
he/him/his
Primary language English
CreditAttribution: nedjo commented
FileSize
2657274-7.patch5.58 KB
interdiff.txt3.08 KB

Fix the remaining bug. Issue was that we were setting the providing feature only within a test for an assigned package. Instead, we need to do so also in the case where a package has not been assigned.

nedjo’s picture

nedjo
he/him/his
Primary language English
CreditAttribution: nedjo commented
Category: Task » Bug report
Priority: Normal » Major
Status: Needs work » Needs review

Changing to a bug and escalating to major, since the bugs are not only in the tests and will prevent correct setting of feature dependencies.

nedjo’s picture

nedjo
he/him/his
Primary language English
CreditAttribution: nedjo commented
Issue summary: View changes

  • nedjo committed 5259c29 on 
    Issue #2657274 by nedjo: Fix errors in FeaturesManagerTest for inter...
nedjo’s picture

nedjo
he/him/his
Primary language English
CreditAttribution: nedjo commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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