Problem/Motivation

assertAttributeEmpty() is deprecated and will be removed in PHPUnit 9.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#5 3126569-5.patch1.59 KBlongwave
#5 3126569-5-will-fail.patch2.14 KBlongwave
#4 3126569-4.patch2.17 KBmondrake
#2 3126569-2.patch866 bytesmondrake

Comments

mondrake created an issue. See original summary.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Active » Needs review
StatusFileSize
new 3126569-2.patch866 bytes

There's only one test using this method. However, fixing is non-trivial since this method is checking a protected propeorty. In PHPUnit philosophy, this shouldn't be the case. However, there's no public API to get the content of the property being tested, and the only alternative weapon would be to use Reflection.

So it's better pause one moment and address the philosophical question first: shall we extend the API or use Reflection?

Status: Needs review » Needs work

The last submitted patch, 2: 3126569-2.patch, failed testing. View results

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review
StatusFileSize
new 3126569-4.patch2.17 KB

A patch for the API way.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
StatusFileSize
new 3126569-5-will-fail.patch2.14 KB
new 3126569-5.patch1.59 KB

I think we can just simplify the test here.

The last submitted patch, 5: 3126569-5-will-fail.patch, failed testing. View results

daffie’s picture

daffie commented
Status: Needs review » Needs work 
+++ b/core/modules/locale/tests/src/Unit/LocaleTranslationTest.php
@@ -50,8 +50,7 @@ protected function setUp() {
+    $this->assertNull($translation->destruct());

This line is not testing the protected class variable $translations is empty.

Also adding code to the class Drupal\locale\LocaleTranslation, so that we can better test it is to me wrong solution. Do as @mondrake suggested in comment #2, use Reflection. That is what it is for.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

But does it need to actually test that? That is an implementation detail. The test was originally added to ensure that destroying the object doesn't cause a crash, which is proved by the failing patch I posted (which reverts what was done when the test was first added)

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Reviewed & tested by the community

#7, #8 that's the philosophical question...

See https://github.com/sebastianbergmann/phpunit/issues/3339

A test should not depend on private implementation details. It was a bad idea to make a bad testing practice this convenient.

Maybe PHPUnit was too strict in dropping this so fast, but it's more to us now if we want to follow suit with this directionally, or not. Using Reflection is exactly a bypass into implementation details - however sometimes that may be the only option, as some comments on the post indicate (accessing factories, for example).

However, for the issue at hand here that assertion seems superfluous, so I think it's OK to just remove it. An alternative if we do not want to add API would be to throw an Exception if the value of ::$translations is null instead of empty array and capture that with a expectException in the test.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 9.1.x-dev » 9.0.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 0bfda930ab to 9.1.x and b107631272 to 9.0.x. Thanks!

I agree that we shouldn't test internals. We're testing the original error - so that's great.

  • alexpott committed 0bfda93 on 9.1.x 
    Issue #3126569 by mondrake, longwave: Replace usage of...

  • alexpott committed b107631 on 9.0.x 
    Issue #3126569 by mondrake, longwave: Replace usage of...

Status: Fixed » Closed (fixed)

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