EntityResource: Provide comprehensive test coverage for Feed entity

Woot!

This looks excellent. I only have nitpicks :)

1. +++ b/core/modules/rest/tests/src/Functional/EntityResource/Feed/FeedJsonAnonTest.php
   @@ -0,0 +1,23 @@
   +  protected static $mimeType = 'application/json';
   +}
   

   Needs \n in between.

2. +++ b/core/modules/rest/tests/src/Functional/EntityResource/Feed/FeedResourceTestBase.php
   @@ -0,0 +1,187 @@
   +      ->setUrl('/rss.xml')
   ...
   +      ->setWebsiteUrl('http://localhost/rss.xml')
   

   Let's use example.com instead of localhost. When reading this, you might mistakenly think that this is referencing a Drupal URL.

   Also, setUrl is the feed URL. So it should be http://example.com/rss.xml. And setWebsiteUrl() is the feed's website URL. So, http://example.com in this case.

3. +++ b/core/modules/rest/tests/src/Functional/EntityResource/Feed/FeedResourceTestBase.php
   @@ -0,0 +1,187 @@
   +      ->setHash('abcdefg')
   +      ->setEtag('abcdefg')
   

   Let's make these have different values.

4. +++ b/core/modules/rest/tests/src/Functional/EntityResource/Feed/FeedResourceTestBase.php
   @@ -0,0 +1,187 @@
   +    /** @var \Drupal\aggregator\FeedInterface $feed */
   +    $feed = $this->entity;
   

   Unused, can be deleted :)

@gaurav.kapoor: this issue is A) assigned to somebody, B) that somebody is at a Drupal sprint event. It's not cool to jump in here and do rerolls. That looks like issue credit mining, and is extremely annoying ;)

@wimleers i just did the minor fixes which will help in solving this quickly. They didn't require much time. There wasn't any activity from 7-8 hours on this issue. I don't want any credit for this issue.Thanks.

Okay. Sorry then. But there wasn't activity for 7-8 hours … because this was posted very late (almost 1 AM), after which people went to sleep. Not working for 7-8 hours for resting is not unreasonable I think.

Uhm, yes... I was actually just getting some sleep before continuing...

Previous patch actually broke the test. Also part of remark .2 in #6 was missed.

Looks much better :)

For testPatch() we're getting the following unexpected error output:

-{"message":"Unprocessable Entity: validation failed.\ntitle: Title: this field cannot hold more than 1 values.\n"}
+{"message":"Unprocessable Entity: validation failed.\ntitle: Title: this field cannot hold more than 1 values.\nurl.0.value: This value should be of the correct primitive type.\nimage.0.value: This value should be of the correct primitive type.\n"}

i.e. the url.0.value and image.0.value values are incomplete.

So, I did some debugging. Turns out it is because you're still passing relative URLs as the feed URL and the image URL. This causes \Drupal\Core\Validation\Plugin\Validation\Constraint\PrimitiveTypeConstraintValidator::validate() to trigger/add a constraint violation. Those should be actual URLs.

Now testPatch() passes. testPost() gets further, but it fails on the very last assertion. The response is a 422 instead of a 201, with this response body:

{"message":"Unprocessable Entity: validation failed.\ntitle: A feed named Feed Resource Post Test already exists. Enter a unique title.\nurl: A feed with this URL http:\/\/example.com\/feed already exists. Enter a unique URL.\n"}

This is because Feed, unlike every other entity type has a FeedTitle constraint that requires titles to be unique. Which causes the existing generic test coverage to fail with the above error.

#18 is green, yay! :) What we still need now, is HAL test coverage.

There's also @vaplas' review in #16. I addressed some of that already:

1. I disagree with this. We don't want internal: URIs. We want an actual feed you can fetch. Which pretty much by definition is an "external" feed. Otherwise we're aggregating our own content :)
2. I already solved this in #18.
3. I don't see why image is necessary for POSTing? It's not a required field. (In fact, it's populated automatically upon fetching a feed for the first time!)
4. Agreed.
5. Agreed.

So only points 4 and 5 of #16 still need to be addressed.

I contacted @rogierbom :)

@rogierbom replied to my e-mail: he said he was going to look at this again in the next few days.

I cleaned up the short arrays, working on the HAL tests right now.

Doh, removed one comma to many...

You forgot to mark it Needs review, which is why #25 and #26 are not being tested :)

You forgot to add the HAL tests. So \Drupal\Tests\hal\Functional\EntityResource\Feed\FeedHalJsonBasicAuthTest and so on.

@Wim: I didn't forget them, I'm still working on them ;-)

Oh, cool :) I only noticed seconds before I marked this issue RTBC :P

Took a while because of a busy period at work, but here's a re-roll including HAL tests.

#31: no problem :) Thanks for posting this!

There are some small problems in your last patch:

1. +++ b/core/modules/hal/tests/src/Functional/EntityResource/Feed/FeedHalJsonTestBase.php
   @@ -0,0 +1,62 @@
   +/**
   + * @group hal
   + */
   

   We don't want this @group annotation: that'd cause PHPUnit to try to run this as a test if you use php vendor/bin/phpunit -c core --group hal. But it's a base class. So it will fail.

2. +++ b/core/modules/hal/tests/src/Functional/EntityResource/Feed/FeedHalJsonTestBase.php
   @@ -0,0 +1,62 @@
   +    return $normalization + [
   +        '_links' => [
   +          'self' => [
   +            'href' => $this->baseUrl . '/aggregator/sources/1?_format=hal_json'
   +          ],
   +          'type' => [
   +            'href' => $this->baseUrl . '/rest/type/aggregator_feed/aggregator_feed'
   +          ],
   +        ],
   +      ];
   ...
   +    return parent::getNormalizedPostEntity() + [
   +        '_links' => [
   +          'type' => [
   +            'href' => $this->baseUrl . '/rest/type/aggregator_feed/aggregator_feed'
   +          ],
   +        ],
   +      ];
   

   The first line is indented correctly. All subsequent lines are indented too much: they have 2 extraneous leading spaces.

Once those are fixed, this is RTBC!

Re-roll with fixes for Wim's remarks in #32

Heh, #31 failed because of #32.1 :)

This patch looks perfect! Thanks, @rogierbom! :)

Patch still applies to 8.4.x-dev

This is blocking #2835845 per #2835845-17: EntityResource: Provide comprehensive test coverage for BlockContent entity.

+++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
@@ -760,6 +760,8 @@ public function testPost() {
+    // Delete the first created entity in case there is a uniqueness constraint.
+    $this->entityStorage->load(static::$firstCreatedEntityId)->delete();

This is a subtle change to all the other tests. It was added in #18 because of the unique constraint. It changes the amount of entities that will exist as a result of running testPost. It might be okay. Not sure - going to think about it. Leaving at RTBC.

It changes the amount of entities that will exist as a result of running testPost. It might be okay. Not sure - going to think about it.

Yes, it is okay.

Drupal\Tests\hal\Functional\EntityResource\Feed\FeedHalJsonTestBase is failing.

Oh hm… this is interesting. When I applied the #33 patch locally (on a branch where I also have other changes), it applied it to the wrong hunk… but when I'm back on HEAD and apply the patch in #33 again, it does apply to the correct hunk.

Why is this even possible? Because there are two very similar-looking hunks:

    // 201 for well-formed request.
    $response = $this->request('POST', $url, $request_options);
    $this->assertResourceResponse(201, FALSE, $response);
    if ($has_canonical_url) {
…
    // 201 for well-formed request.
    $response = $this->request('POST', $url, $request_options);
    $this->assertResourceResponse(201, FALSE, $response);
    if ($has_canonical_url) {

git appears to not include enough context to be able to unambiguously apply this. Rerolled patch, with interdiff to show what I changed in the locally+wrongly applied patch from #3. But note that #33 contains this:

diff --git a/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
index 53796c8aea..298464cd6d 100644
--- a/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
+++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
@@ -760,6 +760,8 @@ public function testPost() {
 
 
     // 201 for well-formed request.
+    // Delete the first created entity in case there is a uniqueness constraint.
+    $this->entityStorage->load(static::$firstCreatedEntityId)->delete();
     $response = $this->request('POST', $url, $request_options);
     $this->assertResourceResponse(201, FALSE, $response);
     if ($has_canonical_url) {

whereas this contains:

diff --git a/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
index 80d8a06..a55ae92 100644
--- a/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
+++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
@@ -757,6 +757,8 @@ public function testPost() {
 
 
     // 201 for well-formed request.
+    // Delete the first created entity in case there is a uniqueness constraint.
+    $this->entityStorage->load(static::$firstCreatedEntityId)->delete();
     $response = $this->request('POST', $url, $request_options);
     $this->assertResourceResponse(201, FALSE, $response);
     if ($has_canonical_url) {

It's interesting that we're effectively running into a git diff or git apply bug here! That's a first :)

Committed and pushed 2dc6d91 to 8.4.x and 4d67f1b to 8.3.x. Thanks!

diff --git a/core/modules/rest/tests/src/Functional/EntityResource/Feed/FeedResourceTestBase.php b/core/modules/rest/tests/src/Functional/EntityResource/Feed/FeedResourceTestBase.php
index b8eb4b0..f15a5e0 100644
--- a/core/modules/rest/tests/src/Functional/EntityResource/Feed/FeedResourceTestBase.php
+++ b/core/modules/rest/tests/src/Functional/EntityResource/Feed/FeedResourceTestBase.php
@@ -156,7 +156,7 @@ protected function getNormalizedPostEntity() {
       ],
       'description' => [
         [
-          'value' =>  'Feed Resource Post Test Description'
+          'value' => 'Feed Resource Post Test Description'
         ]
       ],
     ];

Fixed coding standards on commit.

Yay!

#2835845: EntityResource: Provide comprehensive test coverage for BlockContent entity is now unblocked :)