Refactor the serialization tests to use a mock serializer

The code looks great - Just ...

+++ b/core/modules/hal/lib/Drupal/hal/Tests/NormalizerTestBase.phpundefined
@@ -51,46 +43,58 @@
+  protected function getMockSerializer($normalizers, $encoders) {
...
+  protected function enableLanguage() {
...
+  protected function createTranslatableTextField() {

@@ -99,44 +103,29 @@ function setUp() {
+  protected function createEntityReferenceField() {

+++ b/core/modules/serialization/lib/Drupal/serialization/Tests/EntityResolverTest.phpundefined
@@ -6,7 +6,11 @@
+class EntityResolverTest extends HalNormalizerTestBase {

@@ -91,9 +73,21 @@ function testUuidEntityResolver() {
+  protected function getMockSerializer($normalizers, $encoders) {

+++ b/core/modules/serialization/lib/Drupal/serialization/Tests/NormalizerTestBase.phpundefined
@@ -18,13 +24,40 @@
+  protected function getMockSerializer($normalizers, $encoders) {
...
+  protected function createTextField() {

These need docblocks.

+++ b/core/modules/serialization/lib/Drupal/serialization/Tests/SerializationTest.phpundefined
@@ -36,7 +39,7 @@ public static function getInfo() {
-    $this->serializer = drupal_container()->get('serializer');
+    $this->serializer = \Drupal::service('serializer');
   }
 
   /**

I think we should use $this->container->get('blah') now.

+++ b/core/modules/hal/lib/Drupal/hal/Tests/DenormalizeTest.phpundefined
@@ -22,6 +22,12 @@ public static function getInfo() {
+  public function setUp() {
+    parent::setUp();
+    $this->enableLanguage();
+    $this->createTranslatableTextField();
+  }

I think this setUp method could move the the base class too?

Hmm, maybe not. As not all tests will want this additional stuff set up. I think this patch is good.

+++ b/core/modules/hal/lib/Drupal/hal/Tests/NormalizerTestBase.phpundefined
@@ -51,18 +43,58 @@
+  /**
+   * Creates a mock serializer for testing.
+   *
+   * @param array $normalizers
+   *   The array of normalizers to instantiate the serializer with.
+   * @param array $encoders
+   *   The array of encoders to instantiate the serializer with.
+   *
+   * @return \Symfony\Component\Serializer\Serializer
+   *   The mock serializer.
+   */

I think this documentation should move to Drupal\serialization\Tests\NormalizerTestBase

+++ b/core/modules/serialization/lib/Drupal/serialization/Tests/EntitySerializationTest.phpundefined
@@ -63,7 +63,7 @@ protected function setUp() {
-    $this->serializer = $this->container->get('serializer');
+    $this->serializer = $this->serializer;

This change looks odd - can't we just remove the line?

+++ b/core/modules/serialization/lib/Drupal/serialization/Tests/NormalizerTestBase.phpundefined
@@ -16,15 +22,48 @@
+  /**
+   * {@inheritdoc}
+   */

This class is extending DrupalUnitTestBase so this can't use @inheritdoc - this is related to the first code snippet and comment in the review

Good points Alex. Let's make those changes.

Needed quite a reroll.

Patch in #12 no longer applies.

Reroll attached.

Thanks for getting this going again, forgot about this one!

A couple of quick points:

1. +++ b/core/modules/serialization/src/Tests/EntityResolverTest.php
   @@ -7,51 +7,16 @@
   +class EntityResolverTest extends HalNormalizerTestBase {
   

   We can't inherit from Hal module in serialization module. I think that is my fault originally...sorry! That was a mistake. We will need to move what we need into NormalizerTestBase maybe, and hal\NormalizerTestBase should extend that. Which I think it already does.

2. +++ b/core/modules/serialization/src/Tests/NormalizerTestBase.php
   @@ -18,14 +24,55 @@
   +    $this->installSchema('system', array('url_alias', 'variable'));
   

   We can remove 'variable' from here. This is likely causing a few of the fails :)

No problem, just trying to help.

Removing the system.variable reference attached.

I don't think I am in a good position of understanding to take this much further.

Removing patch got some core dev update changes in it.

Removing the system.variable reference attached.

Rebased against 8.1.

Let's get this going again.

Oh… ha… ha… ha… I last touched this exactly one year ago.

This massively conflicted with #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method's follow-up: #2824576: Delete old REST test coverage: (Read|Create|Update|Delete)Test, deprecate RESTTestBase.

Rebased as well as I could.

And these things can be removed with just as many tests passing.

It's not at all clear to me what exactly this patch is improving. I'm hoping somebody who was around when this issue was first created can shed some light on how to proceed here.

I'm not sure I agree 100% with the issue. In my mind the plan should be to use a mock serializer if the test can also be converted to a unit test maybe? It seems weird to have a kernel test but create our own mock serializer, and not use the serializer from the container when the tests are (likely) using other stuff from the container. Also, issues like #2751325: All serialized values are strings, should be integers/booleans when appropriate were also removing this mock container anyway. So if anything, maybe it makes sense to unify it the other way now, for kernel tests? The reason the issue just mentioned changed this as you essentially have to change the mock serializer to reflect your actual serialization configuration anyway. So you need to keep two things in sync. They also could behave different if they are not added in the same order etc.. too.

Does that mean this is a won't-fix?

Much of what was in \Drupal\Tests\hal\Kernel\DenormalizeTest() has been removed in #2824576: Delete old REST test coverage: (Read|Create|Update|Delete)Test, deprecate RESTTestBase, because it was obsolete thanks to #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method. I think we should try to move the remainder of what's in NormalizeTest and DenormalizeTest into \Drupal\Tests\hal\Functional\EntityResource\HalEntityNormalizationTrait::assertNormalizationEdgeCases()? Then we could remove these tests altogether. If you agree with that, let's close this issue and open a new one for doing that.

I'm not sure I agree it should be removed, I think it's valuable testing hal behaviour in the hal module :) Looking at #2824576: Delete old REST test coverage: (Read|Create|Update|Delete)Test, deprecate RESTTestBase I'm not sure I agree that coverage should have been removed there either. I'm not a fan of relying on integration tests in another module for testing all the cases here. It does look convenient though, as there is already the assertNormalizationEdgeCases() method for this type of thing. I just don't like the continuous coupling between serialization and REST I guess...

Also, #2751325: All serialized values are strings, should be integers/booleans when appropriate will remove most of the existing mocked serializer code anyway.

I just don't like the continuous coupling between serialization and REST I guess...

Me neither, but I had to be practical. We need to test different formats being supported by REST. Those tests need to be thorough. OTOH, the HAL module's test coverage was very thin, plus they were kernel tests, not unit tests.

I do agree with you, which is why there is #2845384: Every normalizer must have strict test coverage! That's all about testing all cases of normalizers, in isolation, i.e. in unit tests. They should test all edge cases.

The problem with the HAL tests (the existing ones, but also the ones that were deleted in favor of test coverage added in #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method) is that they are testing an unhelpful mish-mash of things. They're semi-integration tests, and very incomplete ones at that, only testing a very small set of entity types, and then only testing the happy path.

Per @damiankloip's comment in #31, who is the Serialization module maintainer and was working on all this when this issue was opened, I'm closing this issue.

Because the original issue scope+intent are not relevant anymore. And that we need to further improve the test coverage in the HAL module is blindingly obvious when you look at the code base. When somebody has the time + energy, they should create a new issue. This old one is merely distracting.