Comments

naveenvalecha created an issue. See original summary.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Keenegan’s picture

Assigned: Unassigned » Keenegan
Keenegan’s picture

Assigned: Keenegan » Unassigned

I can't make it work.

I can't even run a previous working test. For example :

php core/scripts/run-tests.sh --url http://drupalvm.dev  --verbose --class "Drupal\Tests\rest\Functional\EntityResource\Block\BlockJsonAnonTest"

returns

    Drupal\Tests\rest\Functional\EntityResource\Block\BlockJsonAnonTest::testGet
    Failed asserting that Array &0 (
        0 => 'https://drupal.org/link-relations/delete-form'
    ) is identical to Array &0 (
        0 => 'https://drupal.org/link-relations/delete-form'
        1 => 'edit-form'
        2 => 'https://drupal.org/link-relations/enable'
        3 => 'https://drupal.org/link-relations/disable'
    ).

with a clean drupalvm install. Is it normal ?

vaplas’s picture

Thank you for wanting to help @Keenegan! You can set core/phpunit.xml.dist (copy to /sites folder for avoid changes in core files) and run from root folder like:

#Unix
php vendor/bin/phpunit --configuration sites/phpunit.xml.dist core/modules/rest/tests/src/Functional/EntityResource/Block/BlockJsonAnonTest.php

#Windows
php vendor/phpunit/phpunit/phpunit --configuration sites/phpunit.xml.dist core/modules/rest/tests/src/Functional/EntityResource/Block/BlockJsonAnonTest.php

See also post and Running PHPUnit tests documentation.

Keenegan’s picture

Thanks for the reply !

I've tried what Wim Leers said in your post, but I have the exact same output :

vagrant@drupalvmRest:/var/www/drupalvm/drupal/web$ php vendor/bin/phpunit -c core core/modules/rest/tests/src/Functional/EntityResource/Block/
PHPUnit 4.8.35 by Sebastian Bergmann and contributors.

F...F...F...

Time: 2.54 minutes, Memory: 6.00Mb

There were 3 failures:

1) Drupal\Tests\rest\Functional\EntityResource\Block\BlockJsonAnonTest::testGet
Failed asserting that Array &0 (
    0 => 'https://drupal.org/link-relations/delete-form'
) is identical to Array &0 (
    0 => 'https://drupal.org/link-relations/delete-form'
    1 => 'edit-form'
    2 => 'https://drupal.org/link-relations/enable'
    3 => 'https://drupal.org/link-relations/disable'
).

/var/www/drupalvm/drupal/web/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php:440

2) Drupal\Tests\rest\Functional\EntityResource\Block\BlockJsonBasicAuthTest::testGet
Failed asserting that Array &0 (
    0 => 'https://drupal.org/link-relations/delete-form'
) is identical to Array &0 (
    0 => 'https://drupal.org/link-relations/delete-form'
    1 => 'edit-form'
    2 => 'https://drupal.org/link-relations/enable'
    3 => 'https://drupal.org/link-relations/disable'
).

/var/www/drupalvm/drupal/web/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php:440

3) Drupal\Tests\rest\Functional\EntityResource\Block\BlockJsonCookieTest::testGet
Failed asserting that Array &0 (
    0 => 'https://drupal.org/link-relations/delete-form'
) is identical to Array &0 (
    0 => 'https://drupal.org/link-relations/delete-form'
    1 => 'edit-form'
    2 => 'https://drupal.org/link-relations/enable'
    3 => 'https://drupal.org/link-relations/disable'
).
vaplas’s picture

Sorry for delay with answer. Unfortunately, I have poor experience with Unix, DVM and manual run of tests (#5 from Windows console and run from PhpStorm works to me). May be interference creates by firewall, antivirus or other network settings. Perhaps launching tests will be the hardest part of this issue :)

Wim Leers’s picture

Works fine here:

wim.leers at acquia in ~/Work/d8 on 8.4.x*
$ php vendor/bin/phpunit -c core core/modules/rest/tests/src/Functional/EntityResource/Block/BlockJsonAnonTest.php
PHPUnit 4.8.27 by Sebastian Bergmann and contributors.

....

Time: 17.04 seconds, Memory: 6.00Mb

OK (4 tests, 57 assertions)

Something must be wrong with your environment setup.

jamesdesq’s picture

It's possible, as ever, that I've missed something here, but when I run the functional block tests (both for the HAL module and the REST module) I get twelve passes each time:

vagrant:///Users/me/VagrantBoxes/vdd/usr/bin/php /home/vagrant/.phpstorm_helpers/phpunit.php --configuration /var/www/drupal8/core/phpunit.xml /vagrant/data/drupal8/core/modules/hal/tests/src/Functional/EntityResource/Block
Testing started at 17:03 ...
PHPUnit 4.8.35 by Sebastian Bergmann and contributors.

Time: 3.25 minutes, Memory: 6.75MB

OK (12 tests, 176 assertions)

Process finished with exit code 0

vagrant:///Users/me/VagrantBoxes/vdd/usr/bin/php /home/vagrant/.phpstorm_helpers/phpunit.php --configuration /var/www/drupal8/core/phpunit.xml /vagrant/data/drupal8/core/modules/rest/tests/src/Functional/EntityResource/Block
Testing started at 17:10 ...
PHPUnit 4.8.35 by Sebastian Bergmann and contributors.

Time: 3.09 minutes, Memory: 6.50MB

OK (12 tests, 176 assertions)

Process finished with exit code 0

Are we actually referring to the data available at the /entity/block_content_type/{block_content_type} endpoint?

jamesdesq’s picture

So the the endpoint to get the default-installed Basic Block would be /entity/block_content_type/basic?_format=json and the entity returned would look like this:

{
uuid: "9b60112e-15d5-42c7-ad03-81910c3211d5",
langcode: "en",
status: true,
dependencies: [ ],
_core: {
default_config_hash: "zglzjmYxi0G0ag9MZ02y0LSJOdpWRwJxyP_OvFojFyo"
},
id: "basic",
label: "Basic block",
revision: 0,
description: "A basic block contains a title and a body."
}

Which I'm guessing is what should be returned by the getNormalizedEntity function in a BlockContentTypeResourceTestBase class?

I'm going to assign this to myself if that's cool with everyone?

jamesdesq’s picture

Assigned: Unassigned » jamesdesq
vaplas’s picture

Cool for me! (Although it is a pity that it failed to help @Keenegan)

#9: 12 tests = 4 tests * 3 test files - all right.

#10: I still have not won, trying to guess what will return the getNormalizedEntity function :) But it's not necessary to play in this game, it's much easier to sacrifice one failed test with return empty array from getNormalizedEntity. And the log will contain what should be returned ;)

But first we need to create BlockContentType entity in createEntity() of course. The second half of the structure obtained by you in #10 will be very useful for this:

id: "basic",
label: "Basic block",
revision: 0,
description: "A basic block contains a title and a body."

Based on these data or some existing code in drupal core like BlockContentType::create( (lite hack!) we can create the right entity. Example:

$block_content_type = BlockContentType::create([
      'id' => 'pascal',
      'label' => 'Pascal',
      'revision' => FALSE,
      'description' => "Provides a competitive alternative to the 'basic' type",
    ]);
Wim Leers’s picture

Yep, thanks for taking this on, @jamesdesq! :)

jamesdesq’s picture

@vaplas - thanks - as always, for your help - that's really really useful.

It's been a bank holiday weekend here in the UK so I've mostly been in the pub, but I've got some time today so with any luck I'll have a first try up in a couple of hours.

jamesdesq’s picture

OK, so one question (sorry, always with the questions!)...

I'm not sure I fully understand what you mean in #12 when you say the log will contain what should be returned.

I've got the REST tests up and running (actually 3/4 of them are redundant, since the block_content_type endpoint only implements get, but it looks as though those tests pass - I'm assuming since it's an instance of ConfigEntityInterface and so gets caught by the first condition in testPost, testPatch and testDelete).

The first part of my BlockContentTypeResourceTestBase looks like this. I've tried explicitly logging the $block_content_type variable, but nothing seems to show up in the logs. Is that what you meant, or have I misunderstood? (FTR, I also tried returning an empty array from the getExpectedNormalizedEntity, which is what I initially thought you meant).

<?php

namespace Drupal\Tests\rest\Functional\EntityResource\BlockContentType;

use Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase;
use Drupal\block_content\Entity\BlockContentType;

abstract class BlockContentTypeResourceTestBase extends EntityResourceTestBase {

  /**
   * {@inheritdoc}
   */
  public static $modules = [
    'block',
    'block_content'
  ];

  /**
   * {@inheritdoc}
   */
  protected static $entityTypeId = 'block_content_type';

  protected $entity;

  /**
   * {@inheritdoc}
   */
  protected function setUpAuthorization($method) {
    $this->grantPermissionsToTestedRole(['administer blocks']);
  }

  protected function createEntity() {
    $block_content_type = BlockContentType::create([
      'id' => 'pascal',
      'label' => 'Pascal',
      'revision' => FALSE,
      'description' => "Provides a competitive alternative to the 'basic' type",
    ]);

    $block_content_type->save();

    \Drupal::logger('rest_test')->notice($block_content_type);

    return $block_content_type;

  }

  protected function getExpectedNormalizedEntity() {
    // return array containing what should be returned

  }
vaplas’s picture

Opps, sorry for bit misunderstanding with 'log' term of #12. And do not forget that I'm not a profy, and my comments should be treated with a bit of skepticism :) Feel free to ask again.

I meant the following:

protected function getExpectedNormalizedEntity() {
  return [];
}

And run for example BlockContentTypeJsonAnonTest.php. And testGet() will failed with info (log) about difference between exprected array and our empty array. It is what we need.


Yes, it may seem strange way, because we use the result instead of checking the result. But it is ok now, because the system is healthy, and we simply reinforce it with additional rest tests to prevent regression in the future ;)
Wim Leers’s picture

@jamesdesq By the way, feel free to post imperfect, in-progress patches. We all do that. It helps others to see how far along this is, and provide suggested changes to help you move quicker :)

jamesdesq’s picture

Thanks both. This is all super useful stuff to know.

So, this seems to pass locally for me. Could someone else have a try please?

vaplas’s picture

Status: Active » Needs review

Looks good! Mr. Bot, will you please.

Status: Needs review » Needs work

The last submitted patch, 18: entityresource_provide-2843774-18.patch, failed testing.

vaplas’s picture

Nice! Only one nit error:

+++ b/core/modules/rest/tests/src/Functional/EntityResource/BlockContentType/BlockContentTypeJsonCookieTest.php
@@ -0,0 +1,29 @@
+class BlockJsonCookieTest extends BlockContentTypeResourceTestBase {

Need BlockContentTypeJsonCookieTest extends... And yes, it's really pass locally, but Mr. Bot loaded all of the classes, which helped to discover it.

+ a bit cleanup:

  1. +++ b/core/modules/rest/tests/src/Functional/EntityResource/BlockContentType/BlockContentTypeResourceTestBase.php
    @@ -0,0 +1,104 @@
    +  public $uuid;
    ...
    +    $this->uuid = $block_content_type->uuid();
    ...
    +      'uuid' => $this->uuid,
    

    better use $this->entity->uuid instead of create $uuid.

  2. +++ b/core/modules/rest/tests/src/Functional/EntityResource/BlockContentType/BlockContentTypeResourceTestBase.php
    @@ -0,0 +1,104 @@
    +  public static $modules = [
    +    'block',
    +    'block_content'
    +  ];
    

    module 'block' not necessary for testing block_content_type, hence $modules = ['block_content'];.

  3. +++ b/core/modules/rest/tests/src/Functional/EntityResource/BlockContentType/BlockContentTypeResourceTestBase.php
    @@ -0,0 +1,104 @@
    +  protected $entity;
    

    Need doc comment @var \Drupal\block_content\BlockContentTypeInterface.

  4. +++ b/core/modules/rest/tests/src/Functional/EntityResource/BlockContentType/BlockContentTypeResourceTestBase.php
    @@ -0,0 +1,104 @@
    +  protected function getExpectedCacheContexts() {
    ...
    +  protected function getExpectedUnauthorizedAccessMessage($method) {
    

    This functions can be removed, because it turned out the same as and by default.

gaurav.kapoor’s picture

Status: Needs work » Needs review
FileSize
8.23 KB
2.42 KB
jamesdesq’s picture

Status: Needs review » Active
FileSize
8.05 KB

Like this?

Also, who is Mr Bot? And how do I get him to run my tests?

jamesdesq’s picture

Status: Active » Needs review
gaurav.kapoor’s picture

Sorry , i didn't see issue is assigned to @jamesdesq.Setting it at needs review will get Mr Bot to run tests.

jamesdesq’s picture

FileSize
2.93 KB

Forgot to interdiff, sorry...

Thanks for your patch, @gaurav.kapoor! I think yours and mine are just about the same - but it doesn't look as though you deleted the unnecessary uuid variable in the BlockContentTypeResourceTestBase.

The last submitted patch, , failed testing.

Wim Leers’s picture

Status: Needs review » Needs work

Testbot reports one coding standards violation in its test results. Could you please fix that?

Other than that detail, this is ready!

vaplas’s picture

FileSize
88.03 KB

#23 absolutely!

+++ b/core/modules/rest/tests/src/Functional/EntityResource/BlockContentType/BlockContentTypeResourceTestBase.php
@@ -0,0 +1,70 @@
+      'uuid' => $this->entity->uuid(),

uuid() is really a method (not property), good correction, thanks!

#29: indeed! To get more info about testbot reports coding standards, click on the green bar under the #23 patch. Bit wait while page load, and click by 1 coding standards message (yes, cursor still default view). Screenshot.

jamesdesq’s picture

Here it is with the coding violation corrected (I'd do an interdiff but I kind of figure for a single newline it's probably not worth it).

Thanks for the explanation, @vaplas!

jamesdesq’s picture

Status: Needs work » Needs review
Wim Leers’s picture

@jamesdesq Can you please provide an interdiff? See https://www.drupal.org/documentation/git/interdiff.

jamesdesq’s picture

Should the interiff be between #31 and #23? Or between #31 and #18? I kind of felt like the difference between #31 and #23 was so small it wasn't really worth it, but I'm happy to do it. Or am I missing the point?

vaplas’s picture

Between #23 and #31. Small interdiff - nice interdiff! (regardless of its triviality). This is really useful for self-control and for simplicity of review.

Wim Leers’s picture

#34: the point of interdiffs is to make it easier for reviewers. Right now, I have to re-read the entire patch, because I don't know which changes you made. Perhaps you changed only what was asked for, perhaps you changed more. An interdiff answers that with much more clarity: it shows exactly what changed between patches.
We all make mistakes. Interdiffs help catch those mistakes earlier :)

jamesdesq’s picture

FileSize
422 bytes

Those are all good reasons :)

Here's the interdiiff!

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Thank you!

vaplas’s picture

Status: Reviewed & tested by the community » Needs review

One bit remark:

+++ b/core/modules/rest/tests/src/Functional/EntityResource/BlockContentType/BlockContentTypeResourceTestBase.php
@@ -0,0 +1,71 @@
+  /**
+   * @var \Drupal\block_content\Entity\BlockContentType
+   */
+  protected $entity;

we use *Interface (see #21.3) for description entity in all other EntityResourse tests. Maybe it is a pattern to make possible to extend (override) the class without rewriting the doc for $entity.

Change status to NR for review this.

Wim Leers’s picture

#39: hah, well-spotted! :) I'm impressed by how fast you became so proficient in rolling patches, doing reviews and doing mentoring, @vaplas!

vaplas’s picture

Status: Needs review » Needs work

Thank you very match for pretty words, @Wim Leers! After your adorable guidance in the adjacent issues, it would be strange to get another result. I'm very grateful to you for this, and I hope that @jamesdesq will also soon become a Jedi power!)

Change to Needs work to fix the type of entity.

jamesdesq’s picture

So the final version should look like this, I think...

Once again, thanks both for all the help and encouragement on this, it's been great (though I think I'm a way of Jedi-ness just yet). I'm hoping to get some more tickets from this group done over the next few days!

jamesdesq’s picture

Status: Needs work » Needs review
FileSize
728 bytes

Sorry, I thought I'd uploaded the interdiff but it didn't seem to work

Status: Needs review » Needs work

The last submitted patch, 43: interdiff-2843774-42.patch, failed testing.

vaplas’s picture

Thank you, @jamesdesq!

Unfortunately, the mechanism for run tests is not ideal, so in such situations we just re-upload the patch again. Like now :)

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

I'm hoping to get some more tickets from this group done over the next few days!

Woot! Still plenty to go in #2824572: [PP-12] Write EntityResourceTestBase subclasses for every other entity type. :)

This now looks ready again.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed f170a96 to 8.4.x and 4511622 to 8.3.x. Thanks!

I've backported to 8.3.x because it is tests only.

I credited @gaurav.kapoor because they supplied a patch which for some reason d.o doesn't see. @Keenegan thanks for trying to review and contribute to the issue. I'm hope you've got phpunit set up and working now.

  • alexpott committed f170a96 on 8.4.x
    Issue #2843774 by jamesdesq, vaplas: EntityResource: Provide...

  • alexpott committed 4511622 on 8.3.x
    Issue #2843774 by jamesdesq, vaplas: EntityResource: Provide...

Status: Fixed » Closed (fixed)

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