Follow-up to #2722731: Mark MissingDependentModuleUnitTest as incomplete
Follow-up to #2721355: MissingDependentModuleUnitTest has the wrong namespace

Problem/Motivation

This will I assume be expected functionality for feature parity coming from simpletest unit tests. This is related to #1273478: Implement @requires and @dependencies within TestBase, mark tests as skipped but implements the phpunit side only and fulfills the existing @todo in KernelTestBase.

Proposed resolution

Use @requires module modulename annotation at the class level and also at the test method level.

Disallow TestDiscovery from discarding tests that are annotated in this way.

Tests annotated this way will scan for the module on the filesystem and mark the test as skipped if the module is not present.

Remaining tasks

User interface changes

API changes

Does not change the API, but alters its behavior to make @require dependencies explicit.

Change record: https://www.drupal.org/node/2857979

Data model changes

CommentFileSizeAuthor
#75 2728579_75.patch18.71 KBMile23
#69 interdiff.txt8.14 KBMile23
#69 2728579_69.patch18.67 KBMile23
#65 interdiff.txt1.1 KBMile23
#65 2728579_65.patch20.14 KBMile23
#63 2728579_63.patch20.3 KBMile23
#59 interdiff.txt689 bytesMile23
#59 2728579_59.patch18.38 KBMile23
#57 interdiff.txt9.82 KBMile23
#57 2728579_57.patch19.06 KBMile23
#48 interdiff.txt5.42 KBMile23
#48 2728579_48.patch16.96 KBMile23
#43 interdiff39_43.txt757 bytesMunavijayalakshmi
#43 2728579_43.patch16.84 KBMunavijayalakshmi
#39 interdiff.txt2.22 KBMile23
#39 2728579_39.patch16.92 KBMile23
#35 interdiff.txt15.06 KBMile23
#35 2728579_35.patch17.01 KBMile23
#29 interdiff.txt1.67 KBMile23
#29 2728579_29.patch8.34 KBMile23
#27 interdiff.txt2.72 KBMile23
#27 2728579_27.patch8.34 KBMile23
#24 interdiff.txt2.71 KBMile23
#24 2728579_24.patch8.21 KBMile23
#21 interdiff.txt808 bytesMile23
#21 2728579_21.patch8.02 KBMile23
#19 interdiff.txt4.95 KBMile23
#19 2728579_19.patch8.06 KBMile23
#12 support_dependencies-2728579-12.patch4.73 KBneclimdul
#12 support_dependencies-2728579-12.interdiff.txt3.84 KBneclimdul
#8 support_dependencies-2728579-8.patch3.59 KBneclimdul
#8 support_dependencies-2728579-8.interdiff.txt1.95 KBneclimdul
#5 support_dependencies-2728579-5.patch3.46 KBneclimdul
#4 support_dependencies-2728579-4.patch3.78 KBneclimdul
#4 support_dependencies-2728579-4.interdiff.txt534 bytesneclimdul
#2 2728579-1.patch3.78 KBneclimdul
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

neclimdul created an issue. See original summary.

neclimdul’s picture

Component: simpletest.module » phpunit
Assigned: Unassigned » neclimdul
Category: Bug report » Task
Status: Active » Needs review
Parent issue: #2722731: Mark MissingDependentModuleUnitTest as incomplete » #2721355: MissingDependentModuleUnitTest has the wrong namespace
FileSize
3.78 KB

Personally I prefer @requires and its what we reference in an exception in KTB already. Here's an implementation.

Status: Needs review » Needs work

The last submitted patch, 2: 2728579-1.patch, failed testing.

neclimdul’s picture

Status: Needs work » Needs review
FileSize
534 bytes
3.78 KB

Got the namespace wrong.

neclimdul’s picture

dawehner’s picture

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

klausi’s picture

Status: Needs review » Needs work

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?

neclimdul’s picture

Status: Needs work » Needs review
FileSize
1.95 KB
3.59 KB

1) done
2) done
3) stole and modified slightly the line from the other test so it fit in one line.
4) done

Yeah, he we actually cleared that up in IRC but neither of us ever commented here. He did mean BTB and I think the plan is to move the logic to a trait with a helper method that BTB, KTB and what ever else can use in their checkRequirements(). That way each can have unique requirements but also share this functionality. Whether that is done as a follow up or here I don't have strong feelings about I just haven't had time to do either yet.

neclimdul’s picture

Thoughts? This seems pretty important for supporting contrib.

klausi’s picture

Status: Needs review » Needs work
  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.

neclimdul’s picture

1) 2) man I must have been really lazy writing that. definitely thanks.
3) 4) yeah... I don't have a problem with skipped tests honestly but I assume someone is going to want to fail on skips and this would break that. Let me think about how to handle that.

neclimdul’s picture

neclimdul’s picture

Status: Needs work » Needs review

err... forgot the comments. Test changes: Moved them into a fixtures directory outside the test suites where they won't be found. Manually include them and then assert their exceptions.

Because the skip exception is a special exception that could be handled by phpunit and I didn't want the possibility of silently running into any problems with that at some point int the future I test each exception in a try catch directly.

klausi’s picture

Status: Needs review » Needs work

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.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Mile23’s picture

Mile23’s picture

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

Mile23’s picture

Status: Needs work » Needs review
FileSize
8.06 KB
4.95 KB

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.

Status: Needs review » Needs work

The last submitted patch, 19: 2728579_19.patch, failed testing.

Mile23’s picture

Status: Needs work » Needs review
FileSize
8.02 KB
808 bytes

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.

The last submitted patch, 19: 2728579_19.patch, failed testing.

dawehner’s picture

  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?

Mile23’s picture

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

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

Status: Needs review » Needs work

The last submitted patch, 24: 2728579_24.patch, failed testing.

dawehner’s picture

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

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

Mile23’s picture

Title: Support @dependencies or @require module in phpunit kernel tests » Support @require module in phpunit kernel tests
Assigned: neclimdul » Unassigned
Issue summary: View changes
Status: Needs work » Needs review
FileSize
8.34 KB
2.72 KB

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.

klausi’s picture

Status: Needs review » Needs work
  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!

Mile23’s picture

Status: Needs work » Needs review
FileSize
8.34 KB
1.67 KB

OK. :-)

klausi’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

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.

klausi’s picture

Title: Support @require module in phpunit kernel tests » Support @require module in phpunit kernel and browser tests
Status: Needs review » Needs work

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?

alexpott’s picture

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.

alexpott’s picture

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.

Mile23’s picture

Component: phpunit » simpletest.module
Status: Needs work » Needs review
FileSize
17.01 KB
15.06 KB

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.

Mile23’s picture

Mile23’s picture

klausi’s picture

Status: Needs review » Needs work

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!

Mile23’s picture

Status: Needs work » Needs review
FileSize
16.92 KB
2.22 KB
    /**
     * @since Method available since Release 3.6.0
     */
    protected function checkRequirements()

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

Addressed all the things.

Thanks. :-)

Mile23’s picture

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

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

neclimdul’s picture

+++ b/core/modules/simpletest/src/TestDiscovery.php
@@ -189,14 +189,17 @@ public function getTestClasses($extension = NULL, array $types = []) {
+      if ($info['type'] == 'Simpletest') {

Wait why do we care about this?

Mile23’s picture

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.

Munavijayalakshmi’s picture

+++ 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.

klausi’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, everything looking good!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 43: 2728579_43.patch, failed testing.

Mile23’s picture

Status: Needs work » Needs review

Restarting test.

dawehner’s picture

+++ 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.

Mile23’s picture

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

Mile23’s picture

klausi’s picture

Status: Needs review » Needs work

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?

Mile23’s picture

Status: Needs work » Needs review

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.

klausi’s picture

Status: Needs review » Needs work

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.

Mile23’s picture

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
klausi’s picture

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?

Mile23’s picture

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

klausi’s picture

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.

Mile23’s picture

Status: Needs work » Needs review
FileSize
19.06 KB
9.82 KB

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.

Status: Needs review » Needs work

The last submitted patch, 57: 2728579_57.patch, failed testing.

Mile23’s picture

Status: Needs work » Needs review
FileSize
18.38 KB
689 bytes

Neglected to change back TestInfoParsingTest after reverting BrowserTestBaseTest.

Mile23’s picture

Patch still applies. Re-running -test.

Status: Needs review » Needs work

The last submitted patch, 59: 2728579_59.patch, failed testing.

Dane Powell’s picture

Title: Support @require module in phpunit kernel and browser tests » Support @requires module in phpunit kernel and browser tests

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

Mile23’s picture

Status: Needs work » Needs review
FileSize
20.3 KB

Reroll.

Status: Needs review » Needs work

The last submitted patch, 63: 2728579_63.patch, failed testing. View results

Mile23’s picture

Status: Needs work » Needs review
FileSize
20.14 KB
1.1 KB

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

dawehner’s picture

  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?

Mile23’s picture

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.

dawehner’s picture

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.

Mile23’s picture

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.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 69: 2728579_69.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Mile23’s picture

Title: Support @requires module in phpunit kernel and browser tests » Explicitly skip @requires module in phpunit kernel and browser tests
Version: 8.5.x-dev » 8.4.x-dev
Issue summary: View changes
Status: Needs work » Reviewed & tested by the community

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

catch’s picture

Status: Reviewed & tested by the community » Needs work

Needs a re-roll.

Mile23’s picture

Status: Needs work » Needs review
FileSize
18.71 KB

Rerolled.

neclimdul’s picture

Status: Needs review » Reviewed & tested by the community

still looks good to me so back to rtbc.

Wim Leers’s picture

Title: Explicitly skip @requires module in phpunit kernel and browser tests » Explicitly skip @requires module in PHPUnit Kernel and Browser tests

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.

  • catch committed 5f3b66e on 8.5.x
    Issue #2728579 by Mile23, neclimdul, Munavijayalakshmi, klausi, dawehner...
catch’s picture

Status: Reviewed & tested by the community » Fixed

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

  • catch committed 8e7f84a on 8.4.x
    Issue #2728579 by Mile23, neclimdul, Munavijayalakshmi, klausi, dawehner...

Status: Fixed » Closed (fixed)

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