Convert web tests to browser tests for Simpletest module

Given that we still need to maintain the simpletest UI for longer maybe it would make sense to copy these tests. This is just a random though which popped into my head.

After some analysis, here's the result.

First, there's a KTB test hiding in there: #2942633: Move KernelTestBaseTest out of simpletest module

Most of the remaining WTB tests in simpletest module are tests of WTB or the UI form behavior.

Tests where we run tests through the form can be converted to BTB, which we should do because we might need the form into D9, tho likely not. #2750461: Remove Simpletest UI because we don't want to maintain a graphical test runner

The rest of the WTB tests can be left as-is, because the goal is to get rid of WTB. Some of the test class names are ambiguous, and really should be cleaned up so we know what they test, but that should be a very (VERY) low priority. For instance, there's BrowserTest and SimpletestBrowserTest, which cover similar areas, but you couldn't tell what based on the class names.

Attached is a patch which adds @groups to WTB tests and @todos for which tests/methods to move.

Updating IS.

After some work, it turns out that Drupal\simpletest\Tests\UiPhpUnitOutputTest is tricky as a BTB test. It calls simpletest_phpunit_run_command() which exec()s PHPUnit with --printer SimpletestUiPrinter. If you have your phpunit.xml set up to use HtmlOutputPrinter (which you should...), then PHPUnit will use HtmlOutputPrinter instead, failing the test.

So since this test is a WTB test, it bypasses all that by virtue of not running under PHPUnit. So we can't move it to be a BTB test without changing it radically.

But it turns out the reason for the test existing is to make sure the output from the test is filtered to have clickable URLs and so forth. This can be unit tested, so this patch adds the unit test of SimpletestUiPrinter. If that's enough to replace UiPhpUnitOutputTest then we can remove it a future patch here.

The rest is fairly straightforward. Drupal\simpletest\Tests\SimpleTestBrowserTest::testTestingThroughUI() was loading the simpletest UI form twice per each test it ran, so that's about a minute and a half more than it needed. That's fixed.

+++ b/core/tests/Drupal/Tests/Listeners/Legacy/HtmlOutputPrinter.php
@@ -15,7 +15,7 @@ class HtmlOutputPrinter extends \PHPUnit_TextUI_ResultPrinter {
-  public function __construct($out, $verbose, $colors, $debug, $numberOfColumns) {
+  public function __construct($out = null, $verbose = false, $colors = self::COLOR_DEFAULT, $debug = false, $numberOfColumns = 80) {

Oh yeah, there's one other thing.

HtmlOutputPrinter (the Legacy one) didn't have the same constructor signature as the superclass.

This woudn't matter for testing SimpletestUiPrinter if we just mocked the class and bypassed the constructor. However, if you look at HtmlOutputPrinter (the not-Legacy one), you'll see that we use a class_alias() to switch the namespace of the class before it's defined.

I think this confuses the PHPUnit mocking system, because I was unable to mock SimpletestUiPrinter, which inherits from one of these two classes, depending.

So the easy solution is to just add all the constructor defaults, copied from the superclass and new one into being.

Fixed CS and adds @covers annotation.

The question still remains from #5: We could just remove the UI test for the output printer, if we're OK with relying on the unit test. No harm in leaving it in, since it doesn't actually spend 20 seconds generating the test form or any such.

@group simpletest marks it as something we can remove once we remove simpletest, or should we look for something more distinctive to indicate this?

This is a good point, because some of these tests might belong in the tests/Drupal/Core/Tests directory or similar.

PhpUnitErrorTest, SimpletestPhpunitRunCommandTest should be moved out of simpletest in #2641632: Refactor simpletest's *_phpunit_*() (and junit) functions etc. to a class, deprecate

TestDiscoveryTest should get moved in #2863055: Move TestDiscovery out of simpletest module, minimize dependencies

A few other tests don't deal directly with simpletest module concerns, so I've moved them to tests/ or the system module. AssertContentTraitTest was testing the deprecated Drupal\simpletest\AssertContentTraitTest and now it's not.

Someone might be counting on the simpletest group somewhere, and they'd get a false positive if these tests fail, so I left those in. We don't really gain anything by taking them out, and when we finally remove simpletest for real we can remove them.

Fixed nits, too.

This looks really solid.

+++ b/core/modules/simpletest/tests/src/Functional/SimpletestUiTest.php
@@ -0,0 +1,62 @@
+    foreach ($tests as $test) {
+      $edit = [
+        "tests[$test]" => TRUE,
+      ];
+      $this->drupalPostForm($url, $edit, t('Run tests'));
+      $assertion->pageTextContains('0 fails, 0 exceptions');
+    }

Any reason why we don't just post all three at once?

That test method is copied over verbatim from core/modules/simpletest/src/Tests/SimpleTestBrowserTest.php. It seems like you'd want to know which test was the problem without spending time running the other two.

Fair nuff

I think we should also take opportunity to ensure that all the test coverage that we're not moving to phpunit has corresponding tests in phpunit. I can't find the equivalent of \Drupal\simpletest\Tests\TimeZoneTest::testAccountTimeZones in BTB but I think there should be. So let's add that to \Drupal\FunctionalTests\BrowserTestBaseTest::testLocalTimeZone().

Also we should have an equivalent of BrokenSetUpTest since what that is testing - that a broken setUp method doesn't completely delete your site is just as relevant for BTB as it is for simpletest.

I agree \Drupal\simpletest\Tests\BrowserTest, \Drupal\simpletest\Tests\MissingCheckedRequirementsTest, \Drupal\simpletest\Tests\SimpleTestTest, \Drupal\simpletest\Tests\SkipRequiredModulesTest, and \Drupal\simpletest\Tests\WebTestBaseInstallTest don't need to be migrated.

I think we need to migrate the \Drupal\simpletest\Tests\SimpleTestBrowserTest::testUserAgentValidation() test as well as testTestingThroughUI() method. We use the same user agent protections in BTB. There are also assertions in \Drupal\simpletest\Tests\SimpleTestBrowserTest::testInternalBrowser() that should be in BTB about how we prevent access if .htkey is missing or incorrect.

I think \Drupal\simpletest\Tests\SimpleTestInstallBatchTest should have a BTB equivalent.

Do we have lower level testing of \Drupal\simpletest\Tests\InstallationProfileModuleTestsTest?

Converting SimpletestUiTest::testTestingThroughUI() to a BTB test will mean converting it within the simpletest module. That's fine if we're going to maintain the UI form beyond D9.0.0. So if we are going to maintain the UI form beyond D9.0.0, then we want to either replace or otherwise not deprecate the test runner it uses: #2748967: Trigger E_USER_DEPRECATED for BC support in simpletest_run_tests() #2750461: Remove Simpletest UI because we don't want to maintain a graphical test runner

We duplicate testAccountTimeZones() to BrowserTestBaseTest because we're checking BTB and WTB integrating with FunctionalTestSetupTrait.

For the same reason, SimpleTestInstallBatchTest gets left behind and we also add Drupal\FunctionalTests\Core\Test\ModuleInstallBatchTest. Both are tests of base classes using FunctionalTestSetupTrait::installModulesFromClassProperty(). This requires moving simpletest_test module to test_batch_test under system module's fixture modules so it's not dependent on simpletest module.

Still to do: BrokenSetUpTest, testUserAgentValidation. The testInternalBrowser conversion will end up testing our mink integration.

Still to do: BrokenSetUpTest, testUserAgentValidation. The testInternalBrowser conversion will end up testing our mink integration.

Based on that, I'm setting this issue back to needs work.

BrokenSetUpTest, via WebTestBase->isInChildSite(), relies on the behavior of DrupalKernel to set DRUPAL_TEST_IN_CHILD_SITE which looks like this:

    // Indicate that code is operating in a test child site.
    if (!defined('DRUPAL_TEST_IN_CHILD_SITE')) {
      if ($test_prefix = drupal_valid_test_ua()) {
        $test_db = new TestDatabase($test_prefix);
        // Only code that interfaces directly with tests should rely on this
        // constant; e.g., the error/exception handler conditionally adds further
        // error information into HTTP response headers that are consumed by
        // Simpletest's internal browser.
        define('DRUPAL_TEST_IN_CHILD_SITE', TRUE);

So these things should happen here:

1) We should leave BrokenSetUpTest where it is, annotated with @group WebTestBase, because it's a test of whether WTB throws an exception against the host site when using the Simpletest UI form. The last meaningful non-CS change was 2014: #2171683: Remove all Simpletest overrides and rely on native multi-site functionality instead BTB does not throw an exception in such a case.

2) Factor that DRUPAL_TEST_IN_CHILD_SITE behavior out of DrupalKernel::bootEnvironment(), into a separate method that we can deprecate along with WebTestBase: #2969741: Deprecate simpletest within DrupalKernel (DRUPAL_TEST_IN_CHILD_SITE)

3) A little bit boggled that I can't find where anyone tested that BrowserTestBase->setUp()/tearDown() doesn't demolish the host DB either. It doesn't help that many of these tests aren't named for the class they're testing. I think this should also be a follow-up, since there's no test to convert. I'm not filing it yet in hopes that someone knows which tests are doing this.

Added a follow-up: #2981870: Duplicate BrokenSetUpTest for BrowserTestBase

Updated IS to be clear on what else needs to happen.

The current patch looks really good and I agree with the scope in the issue summary. Yes we could do each of step in a separate issue but I see the scope of this issue as sorting out the WebTestBase tests of the Simpletest module and therefore doing these tasks here is in scope.

Great work!

1. +++ b/core/modules/system/tests/modules/test_batch_test/test_batch_test.install
   diff --git a/core/modules/simpletest/tests/src/Traits/TestTrait.php b/core/modules/system/tests/src/Traits/TestTrait.php
   similarity index 84%
   
   similarity index 84%
   rename from core/modules/simpletest/tests/src/Traits/TestTrait.php
   
   rename from core/modules/simpletest/tests/src/Traits/TestTrait.php
   rename to core/modules/system/tests/src/Traits/TestTrait.php
   
   rename to core/modules/system/tests/src/Traits/TestTrait.php
   index eeac4963ec..02ef094bc1 100644
   
   index eeac4963ec..02ef094bc1 100644
   --- a/core/modules/simpletest/tests/src/Traits/TestTrait.php
   
   --- a/core/modules/simpletest/tests/src/Traits/TestTrait.php
   +++ b/core/modules/system/tests/src/Traits/TestTrait.php
   

   I'm confused, why is this in system now?

2. +++ b/core/tests/Drupal/FunctionalTests/BrowserTestBaseTest.php
   @@ -641,14 +641,20 @@ public function testInstall() {
      public function testLocalTimeZone() {
   +    $expected = 'Australia/Sydney';
        // The 'Australia/Sydney' time zone is set in core/tests/bootstrap.php
   -    $this->assertEquals('Australia/Sydney', date_default_timezone_get());
   +    $this->assertEquals($expected, date_default_timezone_get());
    
        // The 'Australia/Sydney' time zone is also set in
        // FunctionalTestSetupTrait::initConfig().
        $config_factory = $this->container->get('config.factory');
        $value = $config_factory->get('system.date')->get('timezone.default');
   -    $this->assertEquals('Australia/Sydney', $value);
   +    $this->assertEquals($expected, $value);
   +
   +    // Test that users have the correct time zone set.
   +    $this->assertEquals($expected, $this->rootUser->getTimeZone());
   +    $admin_user = $this->drupalCreateUser(['administer site configuration']);
   +    $this->assertEquals($expected, $admin_user->getTimeZone());
   

   Nice increase of the test coverage!

3. +++ b/core/tests/Drupal/FunctionalTests/BrowserTestBaseUserAgentTest.php
   @@ -0,0 +1,71 @@
   +    ¶
   

   Nitpick: whitespace error :(

4. +++ b/core/tests/Drupal/FunctionalTests/BrowserTestBaseUserAgentTest.php
   @@ -0,0 +1,71 @@
   +    $system_path = $base_url . '/' . drupal_get_path('module', 'system');
   

   Is there a reason you couldn't create a Url object?

5. +++ b/core/tests/Drupal/FunctionalTests/BrowserTestBaseUserAgentTest.php
   @@ -0,0 +1,71 @@
   +    $this->assertTrue(preg_match('/test\d+/', $this->databasePrefix, $matches), 'Database prefix contains test prefix.');
   

   Note: You can use assertRegExp instead

Back to needs worked based on @dawehner's comment in #22.

This is ready.

1. +++ b/core/modules/simpletest/src/Tests/BrokenSetUpTest.php
   @@ -13,6 +13,8 @@
   + * @group WebTestBase
   

   This will now conflict.

2. +++ b/core/modules/simpletest/tests/src/Functional/InstallationProfileModuleTestsTest.php
   @@ -1,15 +1,15 @@
    /**
     * Verifies that tests bundled with installation profile modules are found.
     *
     * @group simpletest
     */
   -class InstallationProfileModuleTestsTest extends WebTestBase {
   +class InstallationProfileModuleTestsTest extends BrowserTestBase {
   

   I don't think that this is a simpletest UI test per se. I think we need test coverage to prove that
   

   Verifies that tests bundled with installation profile modules are found.

   We might already have this type of test discovery test but it is worth proving and adding a comment to the WebTestBase if we do. If we don't that we need to convert this to a better test and one outside of the Simpletest module.

   I seem to have asked this before - ie #14.

3. +++ b/core/tests/Drupal/Tests/Core/Test/AssertContentTraitTest.php
   @@ -1,17 +1,13 @@
    /**
   - * @coversDefaultClass \Drupal\simpletest\AssertContentTrait
   + * @coversDefaultClass \Drupal\KernelTests\AssertContentTrait
   + * @group Test
     * @group simpletest
     */
    class AssertContentTraitTest extends UnitTestCase {
   
   +++ b/core/tests/Drupal/Tests/Core/Test/PhpUnitAutoloaderTest.php
   @@ -1,12 +1,13 @@
    /**
     * Tests that classes are correctly loaded during PHPUnit initialization.
     *
   + * @group Test
     * @group simpletest
     */
    class PhpUnitAutoloaderTest extends UnitTestCase {
   

   Let's remove the @group simpletest here - they are no longer part of simpletest.

I don't think that this is a simpletest UI test per se. I think we need test coverage to prove that

Verifies that tests bundled with installation profile modules are found.
We might already have this type of test discovery test but it is worth proving and adding a comment to the WebTestBase if we do. If we don't that we need to convert this to a better test and one outside of the Simpletest module.

I seem to have asked this before - ie #14.

Going through the list of installation profiles it doesn't look like we have this tested already.

@Lendude Good point. Sorry I also realized I moved the wrong file.

I had a quick chat with @alexpott and we agreed it is good enough to expand the test coverage of TestDiscovery.

And removed the FakeAutoLoader since that doesn't seem to be necessary, we can just pop the ClassLoader in there. Or was that serving a specific testing purpose that I'm overlooking?

Nope, your fix seems totally reasonable.

Strikethrough some more things from IS. Had a quick discussion with @Lendude in slack and it seems like everything from @alexpott's reviews has been addressed so setting it to RTBC.

Committed 06076a3 and pushed to 8.7.x. Thanks!

diff --git a/core/modules/simpletest/tests/src/Unit/TestDiscoveryTest.php b/core/modules/simpletest/tests/src/Unit/TestDiscoveryTest.php
index f62b026ff4..0d2843e97a 100644
--- a/core/modules/simpletest/tests/src/Unit/TestDiscoveryTest.php
+++ b/core/modules/simpletest/tests/src/Unit/TestDiscoveryTest.php
@@ -288,7 +288,6 @@ class FunctionalExampleTest {}
 core: 8.x
 EOF;
 
-
     vfsStream::create([
       'modules' => [
         'test_module' => [
@@ -324,7 +323,7 @@ class FunctionalExampleTest {}
             ],
           ],
         ],
-      ]
+      ],
     ]);
   }
 
@@ -404,7 +403,7 @@ public function testGetTestClassesWithSelectedTypes() {
           'group' => 'example3',
           'type' => 'PHPUnit-Kernel',
         ],
-      ]
+      ],
     ], $result);
   }
 
@@ -424,7 +423,7 @@ public function testGetTestsInProfiles() {
     $test_discovery = new TestDiscovery('vfs://drupal', $class_loader->reveal(), $module_handler->reveal());
 
     $result = $test_discovery->getTestClasses(NULL, ['PHPUnit-Kernel']);
-    $this->assertEquals([
+    $expected = [
       'example3' => [
         'Drupal\Tests\test_profile_module\Kernel\KernelExampleTest4' => [
           'name' => 'Drupal\Tests\test_profile_module\Kernel\KernelExampleTest4',
@@ -432,7 +431,9 @@ public function testGetTestsInProfiles() {
           'group' => 'example3',
           'type' => 'PHPUnit-Kernel',
         ],
-      ]], $result);
+      ],
+    ];
+    $this->assertEquals($expected, $result);
   }
 
   /**

diff --git a/core/tests/Drupal/FunctionalTests/BrowserTestBaseUserAgentTest.php b/core/tests/Drupal/FunctionalTests/BrowserTestBaseUserAgentTest.php
index 3fc6914df4..8468fd5af8 100644
--- a/core/tests/Drupal/FunctionalTests/BrowserTestBaseUserAgentTest.php
+++ b/core/tests/Drupal/FunctionalTests/BrowserTestBaseUserAgentTest.php
@@ -2,7 +2,6 @@
 
 namespace Drupal\FunctionalTests;
 
-use Drupal\Core\Url;
 use Drupal\Tests\BrowserTestBase;
 
 /**
@@ -65,4 +64,5 @@ protected function prepareRequest() {
       $session->setCookie('SIMPLETEST_USER_AGENT', drupal_generate_test_ua($this->databasePrefix));
     }
   }
+
 }

Fixed coding standards.