Class to test deprecation error for deprecated trait triggers deprecation error itself

With which test? Because \Drupal\Tests\serialization\Unit\Normalizer\TimestampItemNormalizerTest doesn't trigger it:

/usr/local/bin/php /private/var/folders/4l/5sndwsz50tqgxjh2525n10br0000gp/T/ide-phpunit.php --configuration /Users/wim.leers/Work/d8/core/phpunit.xml Drupal\Tests\serialization\Unit\Normalizer\TimestampItemNormalizerTest /Users/wim.leers/Work/d8/core/modules/serialization/tests/src/Unit/Normalizer/TimestampItemNormalizerTest.php
#!/usr/bin/env php
PHPUnit 6.5.14 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\serialization\Unit\Normalizer\TimestampItemNormalizerTest
...........                                                       11 / 11 (100%)

Time: 695 ms, Memory: 10.00MB

OK (11 tests, 58 assertions)

Process finished with exit code 0

That's helpful, thanks.

I suspect this is because \Drupal\Tests\serialization\Unit\Normalizer\TimeStampItemNormalizerTraitDeprecatedTestClass exists. And that exists precisely to assert the deprecation error is thrown.

Like I wrote at #2926508-183: Add DateTimeNormalizer+TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON:API:

4. This wasn't very straightforward. In fact, this is the first deprecated trait with explicit deprecation test coverage. A lot of traits have been deprecated, but none of them have explicit test coverage.

This is a first of its kind, and I probably did something wrong. Would love to understand how I should then do this instead. The conclusion will also serve as an example for other deprecated traits.

This is crucial to solve on the way to Drupal 9.

@catch just pointed out that this may be caused by #3031577: \Drupal\Tests\Listeners\DeprecationListenerTrait::getSkippedDeprecations() does not work in unit tests.

Probably the problem is with PHPUnit autoloading, that is loading the deprecated trait (dunno why) and hence the @trigger_error, that sits outside of the trait code, fires.

My thoughts exactly. I've seen similar problems before.

I wonder if it's really necessary to test the deprecation of the trait in isolation. Since that @trigger_error fires on autoloading, the first deprecation test of a deprecated method of the deprecated trait (phew :)) may be intercepted by an @expectedDeprecation of the trait itself?

But nothing in core was using this deprecated trait anymore. That's why I was asked to add an explicit test.

The problem with this is that it is changing the message. It's saying that the trait is not deprecated but a method is.

I could see how it can work if we make it a rule to never deprecate a trait but always deprecate all methods on the trait. But that's still weird and unclear.

I think there's a much simpler option: don't require tests for deprecated traits that only test that the deprecation error is triggered.

The giveaway is this, from the error message:

1x in UnitTestSuite::suite from Drupal\Tests\TestSuites

The test suites are where discovery happens for the phpunit tool. Test suites are not consulted during run-tests.sh, which is why this isn't a fail on the testbot.

I think #12 is correct, because during discovery time TestDiscovery will reflect TimeStampItemNormalizerTraitDeprecatedTestClass, which then loads the trait. Since discovery time is outside of a @group legacy context, it pops up as an outstanding deprecation error.

+1 on #12 because it moves TimeStampItemNormalizerTraitDeprecatedTestClass to a place that is not scanned during discovery but is then autoloadable for the test.

Actually this is a test fix, so we can (should, must...) move this to 8.7.x.

Re: #18: I don't think we should. We're only testing whether the trait triggers the expected error, not classes that use it.

#12 that is genius in its simplicity! I wish I had thought of that :)

Moving back to the actually affected component — thank you phpunit people for taking a look at this 🙏 Once this is committed, we'll have One Correct Example in core :)

Committed and pushed 7555529a7e to 8.8.x and c45d44b3f8 to 8.7.x. Thanks!

As a test only change backported to 8.7.x - I discussed backporting test-only changes with @catch (as a release manager).