Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost’s picture

Issue tags: +Novice

It should be easy find/replace and extend tests

tsphethean’s picture

Status: Active » Needs review
FileSize
2.07 KB

An initial stab at this - getting a little bit stuck on writing a unit test for it so going to keep digging, any pointers welcome!

andypost’s picture

Great! let's check other implementations use parent::buildRow() and then unset()

andypost’s picture

Status: Needs review » Reviewed & tested by the community
andypost’s picture

tsphethean’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

I'm writing tests for this today.

tsphethean’s picture

This issue depends on #2028499: drupal_sort_weight should be converted to a class in order to be able to run unit tests on the EntityListController class

tsphethean’s picture

Managed to get mocking working so that this no longer depends on drupal_sort_weight in common.inc and so should be able to go independently.

tsphethean’s picture

Status: Needs work » Needs review
FileSize
1.97 KB

Added some mocking to the unit tests to remove the dependency on common.inc. Tests passing locally, hopefully good to go.

tsphethean’s picture

Tests passing locally now.

tsphethean’s picture

Tests added and passing locally.

tsphethean’s picture

tsphethean’s picture

not sure what was going on there - patches werent showing as uploaded and then all of a sudden there were 5!

tsphethean’s picture

Patch in #12 is the one to go for review (identical to #11, but prob easier to grab the last patch

tsphethean’s picture

Issue summary: View changes

Updated issue summary.

dawehner’s picture

It would be possible to use data providers to test different kind of labels.

tsphethean’s picture

@dawehner - good point, I've updated the tests to use the data provider from StringTest::providerCheckPlain so that if the String::checkPlain method is ever updated then we'll catch any regressions here.

Status: Needs review » Needs work

The last submitted patch, entitylistcontroller_checkplain-2019071-16.patch, failed testing.

tsphethean’s picture

Assigned: Unassigned » tsphethean

Hmm. So looks like this is something to do with a particular randomString being assigned to the role name when it is generated in simpletest. None of the characters should cause check plain to invalidate. Trying to get a repeatable failure in my local to eliminate this, as I suspect that re-queuing for testing will turn this green without us actually figuring out why and will potentially lead to random failures in future tests.

I'll keep working on this.

dawehner’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests, -Novice

Status: Needs review » Needs work

The last submitted patch, entitylistcontroller_checkplain-2019071-16.patch, failed testing.

tsphethean’s picture

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/tests/Drupal/Tests/Core/Entity/EntityListControllerTest.phpundefined
@@ -0,0 +1,111 @@
+   * @dataProvider Drupal\Tests\Component\Utility\StringTest::providerCheckPlain
...
+  public function testBuildRow($input, $expected, $message, $ignorewarnings = FALSE) {

WOW!

tsphethean’s picture

@dawehner Is that a good wow?

dawehner’s picture

Yes it is.

YesCT’s picture

Issue tags: -Needs tests +RTBC July 1

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/tests/Drupal/Tests/Core/Entity/EntityListControllerTest.phpundefined
@@ -0,0 +1,111 @@
+    // Creates a stub role storage controller and replace the attachLoad()
+    // method with an empty version, because attachLoad() calls
+    // module_implements().
+    $role_storage_controller = $this->getMockBuilder('Drupal\user\RoleStorageController')
+      ->disableOriginalConstructor()
+      ->getMock();
+
+    $module_handler = $this->getMockBuilder('Drupal\Core\Extension\ModuleHandler')
+      ->disableOriginalConstructor()
+      ->getMock();
+
+    $entity_list_controller = $this->getMock('Drupal\Core\Entity\EntityListController', array('buildOperations'), array('user_role', static::$entityInfo, $role_storage_controller, $module_handler));
+
+    $entity_list_controller->expects($this->any())
+      ->method('buildOperations')
+      ->will($this->returnValue(array()));

We can do all of this in setUp() and add a protected $entityListController property to use in the test.

+++ b/core/tests/Drupal/Tests/Core/Entity/EntityListControllerTest.phpundefined
@@ -0,0 +1,111 @@
+    $this->assertEquals($built_row['label'], $expected, $message);

I wonder about the message here... just using the message from the dataprovider seems odd. As we'll get a messages like String::checkPlain() rejects invalid sequence "Foo\\xC0barbaz" see the messages below...

phpunit --filter EntityListControllerTest --tap
TAP version 13
ok 1 - Drupal\Tests\Core\Entity\EntityListControllerTest::testBuildRow with data set #0 ('Foo�barbaz', '', 'String::checkPlain() rejects invalid sequence "Foo\\xC0barbaz"', true)
ok 2 - Drupal\Tests\Core\Entity\EntityListControllerTest::testBuildRow with data set #1 ('�"', '', 'String::checkPlain() rejects invalid sequence "\\xc2\\""', true)
ok 3 - Drupal\Tests\Core\Entity\EntityListControllerTest::testBuildRow with data set #2 ('Fooÿñ', 'Fooÿñ', 'String::checkPlain() accepts valid sequence "Fooÿñ"')
ok 4 - Drupal\Tests\Core\Entity\EntityListControllerTest::testBuildRow with data set #3 ('<script>', '&lt;script&gt;', 'String::checkPlain() escapes &lt;script&gt;')
ok 5 - Drupal\Tests\Core\Entity\EntityListControllerTest::testBuildRow with data set #4 ('<>&"\'', '&lt;&gt;&amp;&quot;&#039;', 'String::checkPlain() escapes reserved HTML characters.')
1..5
tsphethean’s picture

Status: Needs work » Needs review
FileSize
5.8 KB

Thanks for the feedback - updated patch moves set up logic to setup() as suggested.

On the messages, agree they aren't ideal so have added additional statement so it's more obvious what is happening when a test fails. Is there still benefit in re-using the data provider from StringTest::providerCheckPlain or do you think it's more appropriate duplicating the data provider in favour of better messages?

tim.plunkett’s picture

Category: task » bug
Priority: Normal » Major
Status: Needs review » Reviewed & tested by the community

Thanks!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Actually let's not share dataproviders... this is creating an unneeded and difficult to spot dependency on StringTest::providerCheckPlain()

tsphethean’s picture

Status: Needs work » Needs review
FileSize
2.85 KB
6.4 KB

No problem (you're right - the feedback messages do read better now theyre in their own data provider...). New patch attached and what I think is a correctly generated interdiff.

dawehner’s picture

+++ b/core/tests/Drupal/Tests/Core/Entity/EntityListControllerTest.phpundefined
@@ -80,24 +80,22 @@ protected function setUp() {
+   * @dataProvider providerBuildRow

we kind of have a code-pattern to name the providers which is like providerTestBuildRow in this case.

+++ b/core/tests/Drupal/Tests/Core/Entity/EntityListControllerTest.phpundefined
@@ -110,11 +108,28 @@ public function testBuildRow($input, $expected, $check_plain_message, $ignorewar
+   * @see testBuildRow()

let's use self::testBuildRow()

+++ b/core/tests/Drupal/Tests/Core/Entity/EntityListControllerTest.phpundefined
@@ -110,11 +108,28 @@ public function testBuildRow($input, $expected, $check_plain_message, $ignorewar
+

let's just remove the empty line.

tsphethean’s picture

All comments from #31 addressed in this patch - thanks @dawehner.

Status: Needs review » Needs work

The last submitted patch, entitylistcontroller_checkplain-2019071-32.patch, failed testing.

tsphethean’s picture

Odd. All these tests pass in my local. Some of the failures mention out of disk space. could testbot be sick?

Berdir’s picture

Status: Needs work » Needs review
Berdir’s picture

Yes, testbots can get sick. If you see errors like that, try a re-test. And check the testbot number in the event log (this was #953) and ask in IRC if somoene can check it and/or trigger a re-test of the bot to have it auto-disable

tsphethean’s picture

Thanks Berdir

dawehner’s picture

+++ b/core/tests/Drupal/Tests/Core/Entity/EntityListControllerTest.phpundefined
@@ -0,0 +1,134 @@
+ * Tests the entity access controller.
...
+class EntityListControllerTest extends UnitTestCase {

What I really like is to have a @see for the class which is tested.

+++ b/core/tests/Drupal/Tests/Core/Entity/EntityListControllerTest.phpundefined
@@ -0,0 +1,134 @@
+  /**
...
+  protected $role;
...
+  /**
...
+  protected $entityListController;

We sadly have to document this quite easy to read variables.

+++ b/core/tests/Drupal/Tests/Core/Entity/EntityListControllerTest.phpundefined
@@ -0,0 +1,134 @@
+  public function providerTestBuildRow() {

missing @return :(

tsphethean’s picture

Thanks for the review.

What I really like is to have a @see for the class which is tested.

Done

We sadly have to document this quite easy to read variables.

Done

missing @return :(

Done :-)

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/tests/Drupal/Tests/Core/Entity/EntityListControllerTest.phpundefined
@@ -0,0 +1,148 @@
+ * @group Entity

+1 for actually using the groups provided by PHPUnit.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.