Exclude *TestBase.php, *Trait.php and *Interface.php files from test discovery reflection [#2973127]

Problem/Motivation

This is a child issue of #2949363: Impossible to make trigger_error in some files without test fails

In that issue we discovered that we can't perform test discovery on files which @trigger_error() for deprecation without causing false-fail test runs.

These include abstract classes, such as base classes, traits, and interfaces.

The problem: TestDiscovery uses reflection, which loads the file, which triggers the deprecation error whether the class is used or not in any test.

In #2296635: Evaluate performance impact of limiting test discovery to *Test.php filename suffix we considered excluding all files from scan except for *test.php files. This presents BC problems because it could lead to false positives for the unaware.

Proposed resolution

Modify TestDiscovery to exclude reflection on files which end in TestBase.php, Trait.php and Iterface.php.

This mitigates the problem of triggering E_USER_DEPRECATED in the various abstract test types, while also allowing greater BC than simply ignoring all files that don't end in *Test.php.

Map:

#8: version with exclude TestBase.php, Trait.php and Interface.php
#14: version with exclude TestBase.php and Trait.php

Remaining tasks

User interface changes

API changes

Data model changes

Comment	File	Size	Author
#18	2973127-18.patch	3.47 KB	Anonymous (not verified)
#16	2973127-8.patch	3.41 KB	Anonymous (not verified)
#14	interdiff-8-14.txt	2.58 KB	Anonymous (not verified)
#14	2973127-14.patch	3.16 KB	Anonymous (not verified)
#8	interdiff-6-8.txt	1.48 KB	Anonymous (not verified)
#8	2973127-8.patch	3.41 KB	Anonymous (not verified)
#6	interdiff-4-6.txt	3.8 KB	Anonymous (not verified)
#6	2973127-6.patch	3.66 KB	Anonymous (not verified)
#4	interdiff.txt	971 bytes	mile23
#4	2973127_4.patch	2.53 KB	mile23
#3	2973127_3.patch	2.54 KB	mile23

Comments

Comment #1

15 May 2018 at 19:28

Mile23 created an issue. See original summary.

Comment #2

mile23

English

Seattle, WA

commented 16 May 2018 at 18:19

$ ./vendor/bin/phpunit -c core --filter ExpectDeprecationTest
PHPUnit 6.5.8 by Sebastian Bergmann and contributors.

Testing 
..                                                                  2 / 2 (100%)

Time: 13.02 seconds, Memory: 586.50MB

OK (2 tests, 3 assertions)

Remaining deprecation notices (50)

  1x: The Drupal\Tests\hal\Functional\EntityResource\File\FileUploadHalJsonTestBase is deprecated in Drupal 8.6.x and will be removed before Drupal 9.0.0. Instead, use Drupal\Tests\file\Functional\Hal\FileUploadHalJsonTestBase. See https://www.drupal.org/node/2971931.
    1x in FunctionalTestSuite::suite from Drupal\Tests\TestSuites

[ ..repeat x49.. ]

This is because the test suite classes call PHPUnit\Framework\TestSuite::addTestFile() at some point, which performs reflection. We can't really change that.

This means we have to change TestDiscovery::scanDirectory() to exclude *TestBase.php, since the test suite classes use scanDirectory().

Comment #3

mile23

English

Seattle, WA

commented 16 May 2018 at 18:52

Status:

Active

» Needs review

Status	File	Size
new	2973127_3.patch	2.54 KB

Modified scanDirectory() and added a test.

Status	File	Size
new	2973127_4.patch	2.53 KB
new	interdiff.txt	971 bytes

Comment #5

Anonymous (not verified) commented 17 May 2018 at 09:03

Why we need strolower at all?) We expect to here some naming patterns, let's compare them more strictly. This will also help prevent accidental coincidences like TestBasE.php = Bas (bioaktive substanz) + E (vitamin) 🙃

Maybe also include interface.php? Something like:

Edit:

// We don't want to discover abstract TestBase classes, traits, interface. They
// can be deprecated and will call @trigger_error() during discovery.
return
  $current->isFile() &&
  ($file_name = $current->getFilename()) &&
  substr($file_name, -4) === '.php' &&
  substr($file_name, -12) !== 'TestBase.php' &&
  substr($file_name, -9) !== 'Trait.php' &&
  substr($file_name, -13) !== 'Interface.php';

Comment #6

Anonymous (not verified) commented 17 May 2018 at 10:05

Status	File	Size
new	2973127-6.patch	3.66 KB
new	interdiff-4-6.txt	3.8 KB

Comment #7

17 May 2018 at 10:57

Status:

Needs review

» Needs work

The last submitted patch, 6: 2973127-6.patch, failed testing. View results

Comment #8

Anonymous (not verified) commented 17 May 2018 at 11:46

Status:

Needs work

» Needs review

Status	File	Size
new	2973127-8.patch	3.41 KB
new	interdiff-6-8.txt	1.48 KB

Okay, let's remove extra KernelExampleTraIT check, but save 'KernelExampleTest3' to ensure, that we check correct keys.

Comment #9

mile23

English

Seattle, WA

commented 17 May 2018 at 23:56

I limited to *TestBase.php and *Trait.php because that's what @alexpott had mentioned in #28 and #29 in #2949363: Impossible to make trigger_error in some files without test fails

I'm not even sure we have any test interfaces to exclude... Does contrib?

Comment #10

alexpott

he/they

English

🇪🇺🌍

commented 18 May 2018 at 07:20

I think we can now skip this issue a move on doing #2296635: Evaluate performance impact of limiting test discovery to *Test.php filename suffix in a planned fashion since the bug of triggering deprecation messages when scanning all the tests is now fixed.

Comment #11

mile23

English

Seattle, WA

commented 18 May 2018 at 07:27

#2949363: Impossible to make trigger_error in some files without test fails only fixes it for run-tests.sh. This bug still exists for the test suite classes. See #2.

Comment #12

Anonymous (not verified) commented 18 May 2018 at 10:20

Interfaces in tests folder is really rare (7 in core, 20 in contrib)
Interfaces with trigger_error even more rare (4 in core, 3 in contrib)
Interfaces with trigger_error in tests folder - not found.

So this check is based only on the fact that technically the syntax allows you to do this case. Although we have not real cases :) I agree to remove, for the sake of performance.

interfaces in tests folder

#Core
core/tests/Drupal/TestSite/TestSetupInterface.php
core/tests/Drupal/Tests/Component/Plugin/Fixtures/vegetable/VegetableInterface.php
core/modules/system/tests/modules/plugin_test/src/Plugin/plugin_test/fruit/FruitInterface.php
core/tests/Drupal/Tests/Core/Routing/TestRouterInterface.php
core/tests/Drupal/Tests/Core/Field/TestBaseFieldDefinitionInterface.php
core/modules/migrate/tests/src/Kernel/MigrateDumpAlterInterface.php
core/modules/config/tests/config_test/src/ConfigTestInterface.php

#Contrib
/blazy/tests/modules/blazy_test/src/BlazyFormatterTestInterface.php
/blazy/tests/modules/blazy_test/src/BlazySkinTestInterface.php
/blazy/tests/modules/blazy_test/src/Form/BlazyAdminTestInterface.php
/composer/vendor/composer/composer/tests/Composer/Test/Autoload/Fixtures/classmap/SomeInterface.php
/composer/vendor/composer/composer/tests/Composer/Test/Plugin/Mock/CapablePluginInterface.php
/conditional_fields/tests/src/FunctionalJavascript/TestCases/ConditionalFieldCheckedUncheckedInterface.php
/conditional_fields/tests/src/FunctionalJavascript/TestCases/ConditionalFieldFilledEmptyInterface.php
/conditional_fields/tests/src/FunctionalJavascript/TestCases/ConditionalFieldValueInterface.php
/courier/tests/modules/courier_test_message/src/Entity/TestMessageInterface.php
/ctools/tests/modules/ctools_wizard_test/src/ExampleConfigEntityInterface.php
/entity/tests/modules/entity_module_bundle_plugin_test/src/Plugin/BundlePluginTest/BundlePluginTestInterface.php
/graphql/tests/modules/graphql_plugin_test/src/GarageInterface.php
/heisencache/src/Heisencache/tests/MockEventSubscriberInterface.php
/mollom/tests/modules/mollom_test/src/Entity/PostInterface.php
/openapi/tests/modules/openapi_test/src/Entity/OpenApiTestEntityInterface.php
/openapi/tests/modules/openapi_test/src/Entity/OpenApiTestEntityTypeInterface.php
/plugin/tests/modules/plugin_test_helper/src/Plugin/PluginTestHelper/MockPluginInterface.php
/search_api/tests/src/Unit/TestComplexDataInterface.php
/search_api/tests/src/Unit/TestNodeInterface.php
/search_api/tests/src/Unit/TestUserInterface.php

Interfaces with trigger:

#Core
core/lib/Drupal/Core/Routing/RouteFilterInterface.php
core/modules/migrate_drupal/src/Plugin/MigrateCckFieldPluginManagerInterface.php
core/modules/migrate_drupal/src/Plugin/MigrateCckFieldInterface.php
core/lib/Drupal/Core/Routing/EnhancerInterface.php

#Contrib
entity/src/Entity/RevisionableEntityBundleInterface.php
composer/vendor/composer/composer/src/Composer/Package/LinkConstraint/LinkConstraintInterface.php
commerce/src/BundlePluginInterface.php

Comment #13

alexpott

he/they

English

🇪🇺🌍

commented 18 May 2018 at 10:22

I think it is fine to exclude interfaces now - eventually once we fully implement #2296635: Evaluate performance impact of limiting test discovery to *Test.php filename suffix we'll not need this but this is a good halfway step.

Comment #14

Anonymous (not verified) commented 18 May 2018 at 10:31

Status	File	Size
new	2973127-14.patch	3.16 KB
new	interdiff-8-14.txt	2.58 KB

7 files were hidden/shown/deleted

Status	File	Size
hidden	2973127_3.patch	2.54 KB
hidden	2973127_4.patch	2.53 KB
hidden	interdiff.txt	971 bytes
hidden	2973127-6.patch	3.66 KB
hidden	interdiff-4-6.txt	3.8 KB
hidden	2973127-8.patch	3.41 KB
hidden	interdiff-6-8.txt	1.48 KB

#13: Done.

Comment #15

alexpott

he/they

English

🇪🇺🌍

commented 18 May 2018 at 11:08

/me should have been clearer... by

exclude

I meant that excluding interfaces as done in #8 seems fine. Sorry.

Comment #16

Anonymous (not verified) commented 18 May 2018 at 11:17

Title:	Exclude TestBase.php and Trait.php files from test discovery reflection	» Exclude TestBase.php, Trait.php and *Interface.php files from test discovery reflection
Issue summary:	View changes

Status	File	Size
new	2973127-8.patch	3.41 KB

Hah, np) Reupload #8 + updated the scope in Title/IS.

Comment #17

18 May 2018 at 11:19

Status:

Needs review

» Needs work

The last submitted patch, 16: 2973127-8.patch, failed testing. View results

Comment #18

Anonymous (not verified) commented 18 May 2018 at 11:26

Status:

Needs work

» Needs review

Status	File	Size
new	2973127-18.patch	3.47 KB

Opps, reroll.

Comment #19

mile23

English

Seattle, WA

commented 19 May 2018 at 21:34

Issue summary:

View changes

Added a CR: https://www.drupal.org/node/2974044

Updated IS for clarity.

I can't RTBC or I would. :-)

Comment #20

Anonymous (not verified) commented 19 May 2018 at 21:55

Issue summary:

View changes

RTBC++

Comment #21

borisson_

Dutch

Mechelen, 🇧🇪

commented 20 May 2018 at 17:38

Status:

Needs review

» Reviewed & tested by the community

RTBC based on #19 and #20 and the fact that there is sufficient test-coverage.

Comment #22

20 May 2018 at 18:39

Status:

Reviewed & tested by the community

» Needs work

The last submitted patch, 18: 2973127-18.patch, failed testing. View results

Comment #23

alexpott

he/they

English

🇪🇺🌍

commented 21 May 2018 at 09:33

Status:

Needs work

» Reviewed & tested by the community

Using run-tests.sh --list command to test this will a few popular contrib modules lying around. Good news is that core is unaffected. Interestingly the following differences are found:

diff old.txt new.txt
914d913
<  - Drupal\ds\Tests\FastTestBase
1335d1333
<  - Drupal\Tests\field_example\Functional\FieldExampleBrowserTestBase
3331d3328
<  - Drupal\Tests\rules\Unit\Integration\Event\EventTestBase

/**
 * Base test for Display Suite.
 *
 * @group ds
 */
abstract class FastTestBase extends WebTestBase {

/**
 * Class FieldExampleBrowserTestBase.
 *
 * @group field_example
 * @group examples
 */
abstract class FieldExampleBrowserTestBase extends ExamplesBrowserTestBase {

/**
 * Base class for testing Rules Event definitions.
 *
 * @group RulesEvent
 */
abstract class EventTestBase extends RulesEntityIntegrationTestBase {

So this looks great. These are abstract classes that shouldn't be listed and as a result of this change won't be. Nice one.

Comment #24

21 May 2018 at 10:35

Status:

Reviewed & tested by the community

» Needs work

The last submitted patch, 18: 2973127-18.patch, failed testing. View results

Comment #25

mile23

English

Seattle, WA

commented 21 May 2018 at 18:21

Status:

Needs work

» Reviewed & tested by the community

@mixologic put out the fire, so we can go back to RTBC.

Comment #26

22 May 2018 at 04:32

Status:

Reviewed & tested by the community

» Needs work

The last submitted patch, 18: 2973127-18.patch, failed testing. View results

Comment #27

Anonymous (not verified) commented 22 May 2018 at 06:13

Status:

Needs work

» Reviewed & tested by the community

Comment #28

alexpott

he/they

English

🇪🇺🌍

commented 22 May 2018 at 08:44

Category:	Bug report	» Task
Status:	Reviewed & tested by the community	» Fixed

Committed 8f92d9a and pushed to 8.6.x. Thanks!

Only committed to 8.6.x because we shouldn't add new deprecations to 8.5.x so this is really more of a task. Also I'm not sure how long this code will actually hang about about if we do #2296635: Evaluate performance impact of limiting test discovery to *Test.php filename suffix during 8.x - I think we could deprecate support for non *Test.php classes in 8.6.x and remove support in 8.7.x - so this code might only last to 8.7.x - still it is better to have it than not.

Comment #29

22 May 2018 at 08:44

alexpott committed 8f92d9a on 8.6.x

Issue #2973127 by vaplas, Mile23, alexpott: Exclude *TestBase.php, *...

Comment #30

mile23

English

Seattle, WA

commented 22 May 2018 at 14:45

Published CR.

Comment #31

5 June 2018 at 14:49

Status:

Fixed

» Closed (fixed)

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

Exclude *TestBase.php, *Trait.php and *Interface.php files from test discovery reflection

Problem/Motivation

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Comments

Change records for this issue

Exclude TestBase.php, Trait.php and *Interface.php files from test discovery reflection