Problem/Motivation

Let's fix some tests after #2735927: Workbench moderation + field encrypt + translations isn't working got in

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Problem/Motivation

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner created an issue. See original summary.

dawehner’s picture

talhaparacha’s picture

Thank you @dawehner for the patch in #2. As per my review, it fixed the FieldEncryptCacheTest and FieldEncryptProcessEntitiesTest but FieldEncryptTest and EncryptedFieldValueManagerTest were still failing.

Here is another patch which combines all the changes from the patch in #2 along with some additions, and fixes all the tests. It also brings the module back to a stable condition.

Since the module is currently broken (as can be inferred from #2779853: EntityStorageException: Missing bundle for entity type node), I’m raising the priority for this issue.

talhaparacha’s picture

The additional changes in the patch in #3 are explained below:

+++ b/src/Tests/FieldEncryptTest.php
@@ -237,7 +237,7 @@ class FieldEncryptTest extends FieldEncryptTestBase {
-    $this->assertEqual(5, count($encrypted_field_values));
+    $this->assertEqual(10, count($encrypted_field_values));

1. We need to assert for 10 encrypted field values and not 5 in testEncryptFieldTranslation(). That’s because there should be 5 encrypted values for English text along with 5 encrypted values for French text.

+++ b/src/Tests/FieldEncryptTest.php
@@ -246,7 +246,7 @@ class FieldEncryptTest extends FieldEncryptTestBase {
-    // Check if English text is displayed unencrypted.
+    // Check if French text is displayed unencrypted.

2. Typo correction.

+++ b/src/Tests/FieldEncryptTestBase.php
@@ -17,8 +17,6 @@ use Drupal\simpletest\WebTestBase;
- *
- * @group field_encrypt

3. No need for grouping an abstract class.

+++ b/src/EncryptedFieldValueManager.php
@@ -180,7 +180,8 @@ class EncryptedFieldValueManager implements EncryptedFieldValueManagerInterface
-    return $entity->getRevisionId() ?: $entity->id();
+
+    return $entity->getEntityType()->hasKey('revision') ? $entity->getRevisionId() : $entity->id();

4. In #2744887: Fix automatic tests this piece of code:

            if ($entity->getEntityType()->hasKey('revision')) {
	      $revision_id = $entity->getRevisionId();
	    }
	    else {
	      $revision_id = $entity->id();
	    }
	     return $revision_id;

was changed to this for simplification purposes:

         return $entity->getRevisionId() ?: $entity->id();

But on a closer analysis it can be seen that these two code pieces are NOT equivalent. More specifically, the former can return NULL but the latter will never return NULL. This change was causing failures in testEncryptFieldRevision() and hence I’ve tried to correct it.

+++ b/tests/src/Unit/EncryptedFieldValueManagerTest.php
@@ -138,7 +138,7 @@ class EncryptedFieldValueManagerTest extends UnitTestCase {
-      ->setMethods(['getExistingEntity'])
+      ->setMethods(['getExistingEntity', 'getEntityRevisionId'])

5. Additional mocking required because of the change mentioned in last point.

Status: Needs review » Needs work

The last submitted patch, 3: fix_tests_round_2-2764851-3.patch, failed testing.

talhaparacha’s picture

Status: Needs work » Needs review

Because testbot doesn't pull dev versions, but rather "stable" releases so Encrypt module dependency wasn't met.

colan’s picture

Status: Needs review » Reviewed & tested by the community

Works well in my testing.

nerdstein’s picture

Status: Reviewed & tested by the community » Fixed

Pushed to 8.x-2.x

Status: Fixed » Closed (fixed)

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