Problem/Motivation

Followup to [#3128389#31]

Copied from the entire comment here.

+++ b/core/modules/locale/tests/src/Kernel/LocaleTranslationTest.php
@@ -0,0 +1,48 @@
+    // Ensure that all properties on the unserialized object match the original.
+    $translation_reflection = new \ReflectionObject($translation);
+    $unserialized_reflection = new \ReflectionObject($unserialized);
+    // Ignore the '_serviceIds' property in the comparison.
+    $properties = array_filter($translation_reflection->getProperties(), function ($value) {
+      return $value->getName() !== '_serviceIds';
+    });
+    foreach ($properties as $value) {
+      $value->setAccessible(TRUE);
+      $unserialized_property = $unserialized_reflection->getProperty($value->getName());
+      $unserialized_property->setAccessible(TRUE);
+      $this->assertEquals($unserialized_property->getValue($unserialized), $value->getValue($translation));
+    }

This should be removed from the test. It's testing the internals of DependencySerializationTrait that are tested elsewhere. Adding things like this here makes it harder to change the internals. For example - this test unnecessarily impacts #2531564: Fix leaky and brittle container serialization solution

Can we get a follow-up to remove this from 9.1.x and 9.0.x?

Steps to reproduce

Proposed resolution

Remove the indicated code

Remaining tasks

Patch
Review
Commit

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3199428

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

quietone created an issue. See original summary.

quietone’s picture

Issue summary: View changes

clayfreeman made their first commit to this issue’s fork.

clayfreeman’s picture

Status: Active » Needs review
cliddell’s picture

Status: Needs review » Reviewed & tested by the community

Test is passing and still seems to cover the respective test case; RTBC.

alexpott’s picture

Version: 9.2.x-dev » 9.1.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed f6658ba995 to 9.2.x and 9d0be63e2c to 9.1.x. Thanks!

  • alexpott committed f6658ba on 9.2.x
    Issue #3199428 by clayfreeman, quietone: Remove testing the internals of...

  • alexpott committed 9d0be63 on 9.1.x
    Issue #3199428 by clayfreeman, quietone: Remove testing the internals of...

Status: Fixed » Closed (fixed)

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