Explicitly skip @requires module in PHPUnit Kernel and Browser tests

It would be nice to expand this to kernel tests as well. Unit tests don't really make sense I think.

Makes sense! some minor stuff:

1. +++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
   @@ -225,6 +222,49 @@ protected static function getDrupalRoot() {
   +   * Iterates through a list of requires annotations a looks for missing modules
   +   * and skipping the test if any are missing.
   

   Should be "Iterates through a list of requires annotations and looks for missing modules. The test will be skipped if any of the required modules is missing."

2. +++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
   @@ -225,6 +222,49 @@ protected static function getDrupalRoot() {
   +   * @param $annotations
   

   type is missing. I assume string[]?

3. +++ b/core/tests/Drupal/KernelTests/KernelTestBaseTest.php
   @@ -206,6 +206,13 @@ public function testRenderWithTheme() {
      /**
   +   * @requires module drupal_dne_module
   +   */
   +  public function testRequiresModule() {
   +    $this->fail('Running test with missing required module.');
   +  }
   

   This should have a method comment like "Tests that requires annotation work on methods."

   And a long comment after that: "This test method must not be invoked because it requires a not existing Drupal module."

4. +++ b/core/tests/Drupal/KernelTests/MissingDependentModuleUnitTest.php
   @@ -0,0 +1,20 @@
   + * This test should skipped since it requires a module that is not found.
   

   "should be skipped"

@dawehner: this is all about kernel tests? do you mean expanding this to browser tests and javascript browser tests?

1. +++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
   @@ -225,6 +222,50 @@ protected static function getDrupalRoot() {
   +          static::initFileCache();
   +          // drupal_valid_ua() might not be loaded.
   +          require_once $this->root . '/core/includes/bootstrap.inc';
   

   why do we have to do this in every loop iteration? Can this be outside of the loop?

2. +++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
   @@ -225,6 +222,50 @@ protected static function getDrupalRoot() {
   +        } catch (\PHPUnit_Framework_Exception $e) {
   

   there should be a newline after "}".

3. +++ b/core/tests/Drupal/KernelTests/KernelTestBaseTest.php
   @@ -206,6 +206,15 @@ public function testRenderWithTheme() {
      /**
   +   * This method should be skipped since it requires a module that is not found.
   +   *
   +   * @requires module drupal_dne_module
   +   */
   +  public function testRequiresModule() {
   +    $this->fail('Running test with missing required module.');
   +  }
   

   no sure this is a good idea since you will always have a skipped test in the drupal test suite that you cannot satisfy. I would prefer a test that comes with a test class that is not discovered by phpunit, so that it does not even show up in our test suite.

4. +++ b/core/tests/Drupal/KernelTests/MissingDependentModuleUnitTest.php
   @@ -0,0 +1,20 @@
   +/**
   + * This test should be skipped since it requires a module that is not found.
   + *
   + * @group PHPUnit
   + * @requires module drupal_dne_module
   + */
   +class MissingDependentModuleUnitTest extends KernelTestBase {
   

   same problem here, this pollutes our test suite with a skipped test we can never satisfy.

Makes sense! Minor stuff:

1. +++ b/core/tests/Drupal/KernelTests/KernelTestBaseTest.php
   @@ -206,6 +206,34 @@ public function testRenderWithTheme() {
      /**
   +   * @covers ::checkRequirements
   +   * @covers ::checkModuleRequirements
   +   */
   

   doc comment is missing, like "Tests that a test case is skipped when a "requires" annotation is used."

2. +++ b/core/tests/fixtures/MissingDependentModuleMethodTest.php
   @@ -0,0 +1,21 @@
   +use Drupal\Component\FileCache\FileCacheFactory;
   +use Drupal\Core\Database\Database;
   +use org\bovigo\vfs\vfsStream;
   +use org\bovigo\vfs\visitor\vfsStreamStructureVisitor;
   

   unused use statements.

3. +++ b/core/tests/fixtures/MissingDependentModuleMethodTest.php
   @@ -0,0 +1,21 @@
   +
   +class MissingDependentModuleMethodTest extends KernelTestBase {
   

   doc block is missing indicating that this is not a real test case, so that people are not confused why it is never executed.

   Use statement for KernelTestBase is missing.

4. +++ b/core/tests/fixtures/MissingDependentModuleMethodTest.php
   @@ -0,0 +1,21 @@
   +    $this->fail('Running test with missing required module.');
   

   That line can be removed now that the test method is never actually called.

5. +++ b/core/tests/fixtures/MissingDependentModuleTest.php
   @@ -0,0 +1,8 @@
   +namespace Drupal\KernelTests;
   +
   +/**
   + * @requires module drupal_dne_module
   + */
   +class MissingDependentModuleTest extends KernelTestBase {}
   

   use statement for KernelTestBase is missing. And should have a description again that this is not a real test class.

Re-opened #2722731: Mark MissingDependentModuleUnitTest as incomplete with a change.

The patch in #12 still applies.

It should change MissingDependentModuleUnitTest instead of adding MissingDependentModuleTest. If #2722731: Mark MissingDependentModuleUnitTest as incomplete makes it in first then we'll need a re-roll here. If this issue gets committed first, then the other one is duplicate.

Also, TestDiscovery looks at @requires annotation for *all* tests, so run-tests.sh will simply not run the tests here. Check the testbot console log for #12 and there's no evidence that either of the tests in the patch ran and were skipped. https://dispatcher.drupalci.org/job/default/162695/consoleFull

So we need to modify TestDiscovery so it still ignores Simpletest tests based on @requires, but not phpunit ones. And eventually remove that entirely in #1273478: Implement @requires and @dependencies within TestBase, mark tests as skipped

OK, the skip tests are present and passing... https://dispatcher.drupalci.org/job/default/162695/testReport/PHPUnit/Dr...

:-)

Still, though, we need to modify TestDiscovery because the test bypasses the discovery process.

Here's a patch that addresses #14 and also changes TestDiscovery to ignore @requires for PHPUnit-based tests so they can mark themselves as skipped.

We need a follow-up so that run-tests.sh has a way to report skipped tests, because it currently can't.

It turns out that if you use the class hierarchy to determine which class belongs to which suite, you fill up memory. For PHP 5.6 at least. PHP 7 is fine.

Also, getTestInfo() already got this information for us so we can just use that.

1. +++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
   @@ -226,6 +223,51 @@ protected static function getDrupalRoot() {
       */
   +  protected function checkRequirements() {
   +    parent::checkRequirements();
   +
   

   I'm a bit confused that this is part of kernel tests and not browser test base as well?

2. +++ b/core/tests/Drupal/KernelTests/KernelTestBaseTest.php
   @@ -206,6 +206,40 @@ public function testRenderWithTheme() {
   +    $test1->setName('testRequiresModule');
   +    try {
   +      $test1->checkRequirements();
   +      $this->fail('Requirement on method not found.');
   +    }
   +    catch (\PHPUnit_Framework_SkippedTestError $e) {
   +      $this->assertEquals('Module drupal_dne_module is required.', $e->getMessage());
   +    }
   +
   +    $test2 = new MissingDependentModuleTest();
   +    try {
   +      $test2->checkRequirements();
   +      $this->fail('Requirement on class not found.');
   +    }
   +    catch (\PHPUnit_Framework_SkippedTestError $e) {
   +      $this->assertEquals('Module drupal_dne_module is required.', $e->getMessage());
   +    }
   

   Should we maybe split up this test into two parts and then use $this->setExcpectedException?

#23.1 will probably require a trait, and some other refactoring.

This patch splits up the test and uses setExcpectedException().

#23.1 will probably require a trait, and some other refactoring.

I guess adding a follow up would be worth doing maybe?

Added follow-up: #2857977: Support @require module in BrowserTestBase tests

Added change record: https://www.drupal.org/node/2857979

Fixed the failing test... It turns out you have to call setName() in order to tell the test class what method you're dealing with. Then it will pick up the annotations from that method. @neclimdul had done that, and I broke it. Ewps.

1. +++ b/core/tests/Drupal/KernelTests/KernelTestBaseTest.php
   @@ -206,6 +206,52 @@ public function testRenderWithTheme() {
   +    $this->setExpectedException(
   +      \PHPUnit_Framework_SkippedTestError::class,
   +      'Module drupal_dne_module is required.'
   +    );
   

   this should be as close to the call where the exception will occur as possible. Which means right before the last line.

2. +++ b/core/tests/Drupal/KernelTests/KernelTestBaseTest.php
   @@ -206,6 +206,52 @@ public function testRenderWithTheme() {
   +    $this->setExpectedException(
   +      \PHPUnit_Framework_SkippedTestError::class,
   +      'Module drupal_dne_module is required.'
   +    );
   

   same here.

Otherwise looks good!

OK. :-)

Cool, thanks!

I don't get why #2857977: Support @require module in BrowserTestBase tests is not in scope for this issue - it feels more important that KernelTestBase test. Plus if we're going to use traits or whatever - why not get that right first time?

+++ /dev/null
@@ -1,22 +0,0 @@
-class MissingDependentModuleUnitTest extends BrowserTestBase {

I don't get why we are deleting this test. As far as I can see we should be leaving it alone until we completely remove all simpletest.

I'd rather like to get stuff done in smaller chunks - but sure, we can also fold #2857977: Support @require module in BrowserTestBase tests into this issue.

MissingDependentModuleUnitTest is removed because:
a) it is not a unit test
b) is a browser test, not a web test.
c) it is not simpletest specific, so should not be in simpletest module

A trait makes sense to be shared between kernel tests and browser tests. Name? CheckRequirementsTrait? TestRequirementsTrait?

Ah I see about MissingDependentModuleUnitTest - but what this means is that we are missing test coverage of the simpletest code path. Before this change we had test coverage - because all test types when run through run-tests.sh are using the same code - now they are not. So we need to add a simpletest to ensure we are not breaking the legacy code path.

TestRequirementsTrait sounds good.

Also re the BTB being in scope - at the moment it works for them - as the existing test proves when run through run-tests.sh - if we don't include it here then we break it.

BTB behavior was a follow-up because it's easier to write a follow-up than the trait, unless there's sign-off that work should continue on this issue. And now there is.

MissingDependentModuleUnitTest is removed because it can't pass, only fail, isn't very informative. And now it’s replaced by unit tests of TestRequirementsTrait::checkRequirements().

Ah I see about MissingDependentModuleUnitTest - but what this means is that we are missing test coverage of the simpletest code path.

Well, no. We didn't start with coverage of the simpletest path. There are a bunch of linked issues at the top about which test base it should inherit from, with not a lot of consensus, because it's a bad test.

MissingDependentModuleUnitTest inherits from Drupal\Tests\BrowserTestBase and only signals that TestDiscovery should check for dependencies using @requires. Simpletest uses @dependencies (which is converted to a 'requires' array by getTestInfo().)

We don’t want the TestDiscovery path to be testable in this way by a BrowserTestBase test after this issue.

This patch adds a Simpletest based test called \Drupal\simpletest\Tests\SkipRequiredModulesTest that performs this test. Note that it's either skipped or fails based on how it's discovered by run-tests.sh: --class and --file ignore the @dependencies annotation. This will be OK after #1273478: Implement @requires and @dependencies within TestBase, mark tests as skipped but not before.

Also re the BTB being in scope - at the moment it works for them - as the existing test proves when run through run-tests.sh - if we don't include it here then we break it.

By the end of this issue and #1273478: Implement @requires and @dependencies within TestBase, mark tests as skipped, run-tests.sh (really TestDiscovery) won't have an opinion about which type of test it's running, WRT @requires module. It'd be great to have run-tests.sh be properly maintainable and testable, so that we could test the old vs. new behavior, but since it’s not, we don’t.

Looks good, just super minor stuff:

1. +++ b/core/tests/Drupal/Tests/TestRequirementsTrait.php
   @@ -0,0 +1,78 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  protected function checkRequirements() {
   

   @inheritdoc does not really work on traits, I think we just need to copy the docs, right?

2. +++ b/core/tests/fixtures/KernelMissingDependentModuleTest.php
   @@ -0,0 +1,30 @@
   +<?php
   +
   +namespace Drupal\KernelTests;
   +
   +use Drupal\KernelTests\KernelTestBase;
   +
   

   KernelTestBase is in the same namespace, so this use statement is not needed. Also elsewhere. And the coding standards check of the last testrun even detected that, yipee!

    /**
     * @since Method available since Release 3.6.0
     */
    protected function checkRequirements()

Yes, very helpful. :-) Added more helpful docs.

Addressed all the things.

Thanks. :-)

Found this while fixing that last patch: #2861376: ToolkitGdTest uses checkRequirements() incorrectly

Updated change record: https://www.drupal.org/node/2857979

We care about it because we're going to make it go away in #1273478: Implement @requires and @dependencies within TestBase, mark tests as skipped

Simpletest tests don't skip themselves on @requires; they're only skipped on discovery, as if they had never been discovered.

+++ b/core/tests/Drupal/Tests/TestRequirementsTrait.php
@@ -0,0 +1,85 @@
+ * overrides \PHPUnit_Framework_TestCase::checkRequirements(). This
+ * allows the test to be marked as skipped before any kernel boot processes have

Comments do not require a new line until they reach 80 characters.

Fixed and attached new patch.

Thanks, everything looking good!

Restarting test.

+++ b/core/modules/simpletest/src/Tests/SkipRequiredModulesTest.php
@@ -0,0 +1,28 @@
+ * @dependencies simpletest_module_dne

+++ b/core/tests/fixtures/BrowserMissingDependentModuleTest.php
@@ -0,0 +1,30 @@
+ * @requires module drupal_dne_module

+++ b/core/tests/fixtures/KernelMissingDependentModuleTest.php
@@ -0,0 +1,28 @@
+ * @requires module drupal_dne_module

I'd have used the same module name here, but more important, use "does_not_exist" or so. DNE is a an appr. which makes it harder to read than it would have to be.

TLA WTF. (Three-Letter Acronym.)

Also some incorrect docblocks.

Also this needs a follow-up to decide on what group these tests belong to. BTBTest belongs to @group browsertestbase and KTBTest belongs to @group PHPUnit, so basically we can't say 'run all tests about tests' using a group. See also: #2296615: Accurately support multiple @groups per test class

Follow-up: #2864840: Adopt consistent naming standards for multiple test @groups

Nice improvements!

+++ b/core/tests/Drupal/FunctionalTests/BrowserTestBaseTest.php
@@ -244,4 +245,48 @@ public function testInstall() {
+  /**
+   * Tests that a test method is skipped when it requires a module not present.
+   *
+   * In order to catch checkRequirements() regressions, we have to make a new
+   * test object and run checkRequirements() here.
+   *
+   * @covers ::checkRequirements
+   * @covers ::checkModuleRequirements
+   */
+  public function testMethodRequiresModule() {

just realized that this test should not be on BrowserTestBaseTest. It does not use a browser, so this should be a unit or kernel test instead, right?

If the BTB versions of these stub test classes were run under a KTB-based test, then we'd have the following problem:

SIMPLETEST_DB=mysql://thingie ./vendor/bin/phpunit -c core/ --testsuite kernel would attempt to run BTB tests, which would break.

Looked into this, your additions to BrowserTestBaseTest do not test what you think:

$ ../vendor/bin/phpunit tests/Drupal/FunctionalTests/BrowserTestBaseTest.php --filter testMethodRequiresModule
PHPUnit 4.8.35 by Sebastian Bergmann and contributors.

S

Time: 19.79 seconds, Memory: 6.00MB

OK, but incomplete, skipped, or risky tests!
Tests: 1, Assertions: 1, Skipped: 1.

So the test is skipped instead of being fully executed.

So we can't use setExpectedException() for those tests. I tried out this unit test which works:

namespace Drupal\Tests\Core\Test;

use Drupal\FunctionalTests\BrowserMissingDependentModuleMethodTest;
use Drupal\Tests\UnitTestCase;

/**
 * @group Tests
 */
class SkipRequiredModulesTest extends UnitTestCase {

  /**
   * Tests that a test method is skipped when it requires a module not present.
   *
   * In order to catch checkRequirements() regressions, we have to make a new
   * test object and run checkRequirements() here.
   */
  public function testMethodRequiresModule() {
    require __DIR__ . '/../../../../fixtures/BrowserMissingDependentModuleMethodTest.php';

    $test = new BrowserMissingDependentModuleMethodTest();
    $class = new \ReflectionClass(BrowserMissingDependentModuleMethodTest::class);
    $method = $class->getMethod('checkRequirements');
    $method->setAccessible(TRUE);
    
    // We have to setName() to the method name we're concerned with.
    $test->setName('testRequiresModule');
    // We cannot use $this->setExpectedException() because PHPUnit would skip
    // the test before comparing the exception type.
    try {
      $method->invoke($test);
      $this->fail('Missing required module throws skipped test exception.');
    }
    catch (\PHPUnit_Framework_SkippedTestError $e) {
      $this->assertTrue(TRUE, 'Missing required module throws skipped test exception.');
    }
  }

}

So there is no reason to do this in BrowserTestBaseTest which is just slow.

+++ b/core/tests/Drupal/Tests/TestRequirementsTrait.php
@@ -0,0 +1,84 @@
+  private function checkModuleRequirements($root, array $annotations) {
+    // drupal_valid_ua() might not be loaded.
+    require_once $root . '/core/includes/bootstrap.inc';
+    // Scan for modules.
+    $discovery = new ExtensionDiscovery($root, FALSE);
+    $discovery->setProfileDirectories([]);
+    $list = $discovery->scan('module');
+    // Find requirements in the list of modules.
+    foreach ($annotations as $requirement) {
+      if (strpos($requirement, 'module ') === 0) {
+        $module = trim(str_replace('module ', '', $requirement));
+        if (!isset($list[$module])) {
+          throw new \PHPUnit_Framework_SkippedTestError("Module $module is required.");
+        }
+      }
+    }
+  }

minor performance complaint: we should check first if there even is "module " in one of the annoations and only then do the scanning.

Also should we statically cache the scanned module $list once? Not sure how good ExtensionDiscovery already caches stuff.

I tried out this unit test which works:

./vendor/bin/phpunit -c core/ --testsuite unit --filter SkipRequiredModulesTest

PHP Fatal error:  Cannot redeclare drupal_valid_test_ua() (previously declared in /Users/paul/pj2/drupal/core/tests/Drupal/Tests/Core/DrupalKernel/DrupalKernelTest.php:245) in /Users/paul/pj2/drupal/core/includes/bootstrap.inc on line 613

Interesting, but we can fix that by either doing require_once $root . '/core/includes/bootstrap.inc'; in DrupalKernelTest or defining drupal_valid_test_ua() in the same hacky way in our unit test, right?

Or, you know... Keeping suite isolation as per #51. :-)

I think the fact that this is a unit test trumps suite isolation. I agree with catch in #2770549: [plan] Discuss desired API and test suite following deprecation of SimpleTest that a browser test should ideally not invoke any APIs at all and just use the browser. We are really doing a unit test here of our browser testing framework which itself is not a browser test.

OK, so the requirements check for BrowserTestBase works as a kernel test, even though it should actually be a unit test as @klausi points out.

The reason it can't be a unit test is because DrupalKernelTest defines a global function when it shouldn't, because it's a bad unit test. The various solutions to that problem are out of scope here so we'll just have a kernel test.

This patch avoids using BrowserTestBase to test anything, and also does some optimization on the required module discovery per #52.

Neglected to change back TestInfoParsingTest after reverting BrowserTestBaseTest.

Patch still applies. Re-running -test.

Updated title to reflect "requires" instead of "require"

Reroll.

Removed @covers annotation, since this is a functional test and doesn't cover code.

1. +++ b/core/tests/Drupal/KernelTests/Core/Test/BrowserTestBaseTest.php
   @@ -0,0 +1,77 @@
   +    $stub_test = new BrowserMissingDependentModuleMethodTest();
   ...
   +    $stub_test->setName('testRequiresModule');
   +    $class = new \ReflectionClass($stub_test);
   +    $check_requirements = $class->getMethod('checkRequirements');
   +    $check_requirements->setAccessible(TRUE);
   ...
   +    $stub_test = new BrowserMissingDependentModuleTest();
   +    // We have to setName() to the method name we're concerned with.
   +    $stub_test->setName('testRequiresModule');
   +    $class = new \ReflectionClass($stub_test);
   +    $check_requirements = $class->getMethod('checkRequirements');
   +    $check_requirements->setAccessible(TRUE);
   

   Could this test class make checkRequirements public by default? I think this would be a little bit simpler

2. +++ b/core/tests/Drupal/KernelTests/Core/Test/BrowserTestBaseTest.php
   @@ -0,0 +1,77 @@
   +    try {
   +      $check_requirements->invoke($stub_test);
   +      $this->fail('Missing required module throws skipped test exception.');
   +    }
   +    catch (\PHPUnit_Framework_SkippedTestError $e) {
   +      $this->assertEqual('Required modules: module_does_not_exist', $e->getMessage());
   +    }
   ...
   +    try {
   +      $check_requirements->invoke($stub_test);
   +      $this->fail('Missing required module throws skipped test exception.');
   +    }
   +    catch (\PHPUnit_Framework_SkippedTestError $e) {
   +      $this->assertEqual('Required modules: module_does_not_exist', $e->getMessage());
   +    }
   

   Is there a reason we don't use setExpectedException?

1) checkRequirements() is protected because its parent is protected: https://github.com/sebastianbergmann/phpunit/blob/master/src/Framework/T...

2)

+++ b/core/tests/Drupal/KernelTests/Core/Test/BrowserTestBaseTest.php
@@ -0,0 +1,77 @@
+    // We cannot use $this->setExpectedException() because PHPUnit would skip
+    // the test before comparing the exception type.
+    try {
+      $check_requirements->invoke($stub_test);
+      $this->fail('Missing required module throws skipped test exception.');
+    }
+    catch (\PHPUnit_Framework_SkippedTestError $e) {
+      $this->assertEqual('Required modules: module_does_not_exist', $e->getMessage());
+    }

+++ b/core/tests/Drupal/Tests/TestRequirementsTrait.php
@@ -0,0 +1,91 @@
+      if (!empty($not_available)) {
+        throw new \PHPUnit_Framework_SkippedTestError('Required modules: ' . implode(', ', $not_available));
+      }

If the module requirement isn't found, we throw a skipped test exception. PHPUnit handles the exception to skip the whole test before it evaluates any expected exceptions, so the test never passes or fails. We solve this with a try block.

Could this test class make checkRequirements public by default? I think this would be a little bit simpler

Oh well, i was thinking making it public in the test class itself, but just there, so you don't have to do the reflection dance.

So as I sit and look at the last patch I keep asking myself: Why are there two tests for BrowserTestBase? And the answer is: I never finished. :-)

That's why the interdiff is bigger than it should be.

This patch properly tests both BrowserTestBase and KernelTestBase, both as kernel tests.

It also adds public accessors to the fixture test classes, so we can avoid doing the reflection thing as per #68.

So as I sit and look at the last patch I keep asking myself: Why are there two tests for BrowserTestBase? And the answer is: I never finished. :-)

ha, I was wondering, do you maybe need some special setup, which is provided by BTB by default, but it turns out nope :)

+++ b/core/tests/Drupal/KernelTests/Core/Test/BrowserTestBaseTest.php
@@ -0,0 +1,71 @@
+    // We cannot use $this->setExpectedException() because PHPUnit would skip
+    // the test before comparing the exception type.
+    try {

Thank you for having an explanation.

I can't repro the test fail locally, for either 8.4.x or 8.5.x: https://www.drupal.org/pift-ci-job/728283

I'd argue that this is a good candidate for 8.4.0 release, since it's a testing improvement, but it's also somewhat disruptive in that tests will be marked as skipped instead of just ignored by TestDiscovery. Whether that's too disruptive is obviously for maintainers to decide. Setting back to 8.4.x.

It's also not an API change, really. It just changes the behavior of the existing API to give more information to users.

Re-running the test, resetting RTBC.

Updating change record: https://www.drupal.org/node/2857979

Needs a re-roll.

Rerolled.

Has CR, that clearly explains the improvement 👍

1. +++ b/core/tests/Drupal/Tests/TestRequirementsTrait.php
   @@ -0,0 +1,91 @@
   +   * Check module requirements for the Drupal use-case.
   

   Supernit: s/use-case/use case/

2. +++ b/core/tests/Drupal/Tests/TestRequirementsTrait.php
   @@ -0,0 +1,91 @@
   +   * @throws \PHPUnit_Framework_SkippedTestError
   +   *   Thrown when the requirements are not met, and this test should be
   +   *   skipped. Callers should not catch this exception.
   

   ❤️ Love the explicit comment!

3. +++ b/core/tests/fixtures/BrowserMissingDependentModuleTest.php
   @@ -0,0 +1,37 @@
   +class BrowserMissingDependentModuleTest extends BrowserTestBase {
   

   I'd have thought this would be called browserMissingDependentModuleClassTest, but that's nitpicking.

4. +++ b/core/tests/fixtures/KernelMissingDependentModuleTest.php
   @@ -0,0 +1,35 @@
   +class KernelMissingDependentModuleTest extends KernelTestBase {
   

   I'd have thought this would be called KernelMissingDependentModuleClassTest, but that's nitpicking.

Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!

Why the change record is not published?

I arrived to this issue because I'm dealing with a test that is not skipped despite having the annotation.
I think we need to wait for this bug to be fixed before publishing it.
#3261817: TestRequirementsTrait::checkRequirements() does not work as expected

publish the change record