Updated: Comment #N

Problem/Motivation

The two PHPUnit tests in Edit module do not run with the rest of the tests. This is because they are in lib/, when they should be in tests/. Additionally, they are currently failing when run.

Proposed resolution

  1. Move the tests to core/modules/edit/tests/Drupal/edit/Tests/Access
  2. Fix the failing tests. Berdir says "looks like they do tricks for field_access(), which no longer exists, so at least that will need to be updated now"
Files: 
CommentFileSizeAuthor
#8 missed_unit_tests-2108691-08.patch4.13 KBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 58,871 pass(es).
[ View ]
#8 interdiff.txt3.39 KBWim Leers
#7 2108691-07-missed-unit-tests.patch2.59 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] 58,981 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#6 2108691-06-missed-unit-tests.patch1.85 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#1 2108691-01-missed-unit-tests.patch1.85 KBlinclark
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Comments

Status:Active» Needs work
StatusFileSize
new1.85 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Here is a patch (from a paste by timplunkett) which tackles part 1, moving these tests to the correct directory.

Issue tags:+sprint, +Spark

UGH! Great find!

Strange that none of us in #2047659: Add test coverage for edit module access checkers — where these were introduced — noticed this :(

Thanks for pinging me!

Status:Needs work» Needs review

Yeah I just noticed this a couple days ago when reviewing another phpunit test elsewhere. I grepped and edit module seems the only one that has its unit test in the wrong place. I thought then it would be found here, but apparently not :D Thanks for the issue @linclark!

Status:Needs review» Needs work

The last submitted patch, 2108691-01-missed-unit-tests.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.85 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Should be moved to a slightly different place (/tests dir under the module) for the simpletest runner to find it as well. Like all other phpunit tests are in core.

StatusFileSize
new2.59 KB
FAILED: [[SimpleTest]]: [MySQL] 58,981 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Ok, so there are no fails in EditEntityAccessCheckTest locally. The only fail in EditEntityFieldAccessCheckTest is related to the test with a *non-existent field*: testAccessWithNonExistingField. This will pass through the validateAndUpcastRequestAttributes() AFAIS because the field info does have the example field for the entity. *However* the mock entity does not actually have that field, so the current implementation of accessEditEntityField() that is being testing will barf out a method called on non-object:

<?php
 
public function accessEditEntityField(EntityInterface $entity, $field_name) {
    return
$entity->access('update') && $entity->get($field_name)->access('edit');
  }
?>

So the question is whether this method should be more resilient and somehow work even if the field is not actually there, although the request upcasting should already fend those situations off? If this method should stay as-is then the mocks need to be more elaborate and actually add the field on the entity. Not sure how to do that due to my very limited phpunit experience...

Here is a minor update that adds more fault tolerance to this method. It will merely let the tests run without a fatal error. Not the suggested final solution necessarily.

StatusFileSize
new3.39 KB
new4.13 KB
PASSED: [[SimpleTest]]: [MySQL] 58,871 pass(es).
[ View ]

The $empty_field case was nonsensical, and due to the move from field_access() to $entity->get($field_name)->access('edit'), it was no longer $this->fieldInfo that was called when checking edit access for a field, but $entity.

So the solution is actually pretty simple :)

Indeed mocking get() to return the field looks to me one of possible good solutions. :)

Status:Needs review» Reviewed & tested by the community

This looks great. It did not keep *any* of my code additions (which merely helped to pinpoint the issue itself), so I should be fine to RTBC :) The mocking of field seems right. The code also removes the field_access() function which is not used anymore. The field_valid_language() is still used in the code, so needs to stay in tests.

Status:Reviewed & tested by the community» Fixed

Wow, great find. I wonder if there's some way to write a test for missing PHPUnit tests (wow, meta ;)), but I guess not really.

Committed and pushed to 8.x. Thanks!

Issue tags:-sprint

removed the sprint tag

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