Add test coverage for edit module access checkers

What exactly is there to unit test here? All it does is

1. parameter upcasting (which should be handled by the routing system but still is in a very bad shape, hence it had to be reimplemented here, there is a @todo for that)
2. reuse $entity->access() and field_access()

I don't see the point in adding additional tests for this?

We have unit tests for those APIs. But we don't necessarily have integration tests for putting it all together quite the way edit does. That needs testing.

In addition, edit provides a custom access checker. As such, it is unit testable. If you just used an existing access checker, you'd be done, but by writing a custom one, you now have code to be tested.

I agree with the unit tests (and admit fault there!), but I still disagree with the integration tests. Regardless of me disagreeing, I'm providing the test coverage here anyway.

Note that even the routing system's access checking system itself does not have integration tests: #1984528-53: Follow-up: Allow for more robust access checking. dawehner also thinks that's a bad idea; here are relevant excerpts from an IRC discussion about that issue:

dawehner: WimLeers: i don't think an extra webtest is the way to go
WimLeers: dawehner: Interesting  Why? Honestly, I agree with that in principle. But lots of respectable people seem to disagree with that, see e.g. https://drupal.org/node/2047659 and https://drupal.org/node/2031385 forced me to do precisely that for things that already do have unit test coverage!!!!!
…
WimLeers: dawehner: as I said in the comments on the issue, a test would help with people seeing examples like the ones they'd be using
WimLeers: dawehner: but the policy is to have integration tests for this sort of thing too
dawehner: WimLeers: so we test everything both with integration and unit tests? i can't really see where to start and where to stop

Also, via #2031385: Editor's in-place editing AJAX endpoint broken because of #1043198 and routing system bug, I stumbled upon #1984528-53: Follow-up: Allow for more robust access checking, which is how I found that here too, _access_mode: 'ALL' is missing. Due to the change towards static::(ALLOW|DENY), the behavior was changed, which meant that now _access_mode: 'ALL' had to be added, but I wasn't aware of this ALL vs. ANY route option, let alone that the default was ANY.

Even though the default is being fixed at #2063401: Replace the default _access_checks(access mode) with ALL instead of ANY, I believe it's valuable to be explicit about it. So, changing that too in this patch.

So, patch attached that adds

1. integration tests, but a tiny part of that (the checking of the response body) is blocked on #2063303: A Drupal 8 AjaxResponse that must return a 403, returns a 403 and prints "A fatal error occurred: "
2. unit tests, but only for EditEntityAccessCheck, not for EditEntityFieldAccessCheck because that requires me to be able to mock field_(access|info_field|info_instance|valid_language)() and AFAICT those are not yet mockable

@@ -16,15 +16,18 @@ edit_field_form:
+  options:
+    _access_mode: 'ALL'
...
+  options:
+    _access_mode: 'ALL'

Should we better wait until the other issue is in?

@@ -0,0 +1,68 @@
+ * Contains \Drupal\edit\Tests\EditEntityAccessCheckTest.

In general it would be also cool to have the same directory structure in the tests as in the actual code.

@@ -0,0 +1,68 @@
+ * @group Edit
+ */

When I last time talked with someone using phpunit quite a lot his oppinion was to use @group Drupal all the time, so you can execute all of them without the ones from symfony. Note: you can use multiple groups.

@@ -0,0 +1,68 @@
+class EditEntityAccessCheckTest extends UnitTestCase {

Can we have a @see \classNameWithNamespace?

@@ -0,0 +1,68 @@
+    $this->assertEquals($entity_access->appliesTo(), array('_access_edit_entity'), 'Access checker returned the expected appliesTo() array.');

Note: this error is just displayed if the test failed, so we use the reverse logic for the messages in some places ...

@@ -0,0 +1,68 @@
+    $editable_node = $this->getMockBuilder('Drupal\node\Plugin\Core\Entity\Node')
+      ->disableOriginalConstructor()
+      ->getMock();
+    $editable_node->expects($this->any())
+      ->method('access')
+      ->will($this->returnValue(TRUE));
...
+    $readonly_node = $this->getMockBuilder('Drupal\node\Plugin\Core\Entity\Node')
+      ->disableOriginalConstructor()
+      ->getMock();
+    $readonly_node->expects($this->any())
+      ->method('access')
+      ->will($this->returnValue(FALSE));

Do we need a full Node or is just a mocked NodeInterface enough?

@@ -0,0 +1,68 @@
+    $this->assertEquals(TRUE, $access);
...
+    $this->assertEquals(FALSE, $access);

Let's use $this->assertTrue and $this->assertFalse

@@ -0,0 +1,71 @@
+ *
...
+class EditEntityFieldAccessCheckTest extends UnitTestCase {
+

Most of the points applies to that class as well.

Should we better wait until the other issue is in?

I said this: "I believe it's valuable to be explicit about it. So, changing that too in this patch." Do you disagree with that?

In general it would be also cool to have the same directory structure in the tests as in the actual code.

AFAICT none of the existing unit tests don't do that though? I'd like to be consistent with everything else in core.

When I last time talked with someone using phpunit quite a lot his oppinion was to use @group Drupal all the time, so you can execute all of them without the ones from symfony. Note: you can use multiple groups.

Same, nothing is doing that yet, so I don't want to introduce that here.

Can we have a @see \classNameWithNamespace?

Done.

Note: this error is just displayed if the test failed, so we use the reverse logic for the messages in some places ...

I looked at EntityAccessCheckTest as the guiding example. It's exactly the same there. I'm just being consistent.

Do we need a full Node or is just a mocked NodeInterface enough?

Good point! :) I changed it to NodeInterface and it failed.
Also note that this, too, is exactly the same as EntityAccessCheckTest.

Let's use $this->assertTrue and $this->assertFalse

Good catch! Fixed.
Also note that this, too, is exactly the same as EntityAccessCheckTest. I fixed it there too.
And actually, that's not even good enough. Because it should be asserting that the returned value is identical to either of AccessInterface::(ALLOW|DENY|KILL). So I switched it to using $this->assertSame(AccessCheckInterface::(ALLOW|DENY|KILL), $access). So I changed it to that everywhere. (RoleAccessCheckTest was already doing this.)

AFAICT none of the existing unit tests don't do that though? I'd like to be consistent with everything else in core.

I would say that there are just a few UnitTests (not simpletess) which not uses the same directory structure. Feel free to provide some numbers
,but core/tests/Drupal/Tests/Core seriously provides similar structure. EntityAccessCheckTest is just one example

Same, nothing is doing that yet, so I don't want to introduce that here.

Just do it. #2057905-5: [policy, no patch] Discuss the standards for phpunit based tests for a comment about that.

Done.

Thanks

I looked at EntityAccessCheckTest as the guiding example. It's exactly the same there. I'm just being consistent.

Well, I would not assume everything to follow proper standards. They are developed during the time, see also the linked issue above.

Also note that this, too, is exactly the same as EntityAccessCheckTest. I fixed it there too.

Mhhh can't we just make a novice follow up? It makes things more complex to review etc.

And actually, that's not even good enough. Because it should be asserting that the returned value is identical to either of AccessInterface::(ALLOW|DENY|KILL). So I switched it to using $this->assertSame(AccessCheckInterface::(ALLOW|DENY|KILL), $access). So I changed it to that everywhere. (RoleAccessCheckTest was already doing this.)

Good point!

I would say that there are just a few UnitTests (not simpletess) which not uses the same directory structure. Feel free to provide some numbers
,but core/tests/Drupal/Tests/Core seriously provides similar structure. EntityAccessCheckTest is just one example

D'oh — I had misread! I thought you said you wanted it to be in the same directory as the class/file it is testing, but that's not at all what you said. My bad. Fixed.

(I must say that by applying this, the directory structure is now ridiculously deep. From core/modules/edit/lib/Drupal/edit/Tests/TEST.PHP to core/modules/edit/lib/Drupal/edit/Tests/edit/Access/TEST.PHP…)

Just do it.

Just did it! :P

Mhhh can't we just make a novice follow up? It makes things more complex to review etc.

For one new line and one changed line…? Done: #2070045: EntityAccessCheckTest should use AccessCheckInterface::ALLOW.

I guess you are familiar with http://xkcd.com/356/

This still does not cover every case, but at least some of the possible ones.

There we go.

This is better indeed.

Changes:

1. Fixed test failures reported by testbot: changes to logic made in #9 swapped two arguments, hence causing these failures.
2. Fixed and cleaned up tests introduced in #9.
3. Updated EditEntityAccessCheck to be in sync with EditEntityFieldAccessCheck's changes: for consistency.

1+2: see interdiff-test_fixes.txt.
3: see interdiff-apply_similar_tests_to_other_access_checker.txt.

Have not found any issues except one of semantics and unfamiliarity :D

+++ b/core/modules/edit/lib/Drupal/edit/Tests/edit/Access/EditEntityFieldAccessCheckTest.php
@@ -0,0 +1,342 @@
+namespace Drupal\edit\Tests\edit\Access {
+
...
+}
+
+}
+
+// @todo remove once field_access() and field_valid_language() can be injected.
+namespace {

There have been some (for me) strange things in my patch (eg. @group phpdoc), which I grepped for and found they are kind of used already. Discussed this namespace use with Wim and I understand this is needed so a top level function can be defined, so the namespace is not provided for the whole file. It may be a good idea to add a comment to the top to signify this.

There have been some (for me) strange things in my patch (eg. @group phpdoc), which I grepped for and found they are kind of used already. Discussed this namespace use with Wim and I understand this is needed so a top level function can be defined, so the namespace is not provided for the whole file. It may be a good idea to add a comment to the top to signify this.

I won't fight for any additional suggestions, just telling that this is common in some of the phpunit tests which appear more and more over time.

Great additional test coverages and fixes by wim!

+++ b/core/modules/edit/lib/Drupal/edit/Tests/edit/Access/EditEntityAccessCheckTest.php
@@ -0,0 +1,168 @@
+    $route = new Route('/edit/form/node/1/body/und/full', array(), array('_access_edit_entity' => 'TRUE'));
...
+    $route = new Route('/edit/form/node/1/body/und/full', array(), array('_access_edit_entity' => 'TRUE'));
...
+    $route = new Route('/edit/form/node/1/body/und/full', array(), array('_access_edit_entity_field' => 'TRUE'));

+++ b/core/modules/edit/lib/Drupal/edit/Tests/edit/Access/EditEntityFieldAccessCheckTest.php
@@ -0,0 +1,342 @@
+    $route = new Route('/edit/form/node/1/body/und/full', array(), array('_access_edit_entity_field' => 'TRUE'));
...
+    $route = new Route('/edit/form/node/1/body/und/full', array(), array('_access_edit_entity_field' => 'TRUE'));
...
+    $route = new Route('/edit/form/node/1/body/und/full', array(), array('_access_edit_entity_field' => 'TRUE'));
...
+    $route = new Route('/edit/form/node/1/body/und/full', array(), array('_access_edit_entity_field' => 'TRUE'));
...
+    $route = new Route('/edit/form/node/1/body/und/full', array(), array('_access_edit_entity_field' => 'TRUE'));
...
+    $route = new Route('/edit/form/node/1/body/und/full', array(), array('_access_edit_entity_field' => 'TRUE'));

It is good to know that our routes and controllers/access checkers are indepedent from the actual paths, but nevertheless we should at least match the entity type and field we test with.

#15: agreed, fixed. RTBC? :)

D'oh. It'd help if I'd uploaded the right patch and the interdiff.

All right, looks good as per above!

Not yet renamed, so adjusting title.

Awesome, it'll be nice not to ever break this again. :)

Committed and pushed to 8.x. Thanks!

Well, it was never broken :) But now it's guaranteed that won't happen, so yay indeed.