Deprecate hook_test_* hooks in simpletest

I'd really like to get a better understanding for "real world" use-cases for those hooks, because I'm very very tempted to remove all of them altogether without replacement.

It sounds as if you have a use-case — could you enlighten me?

Possible use case : when testing with an alternate path or cache plugin (e.g. mongodb), the data created during the core path.test is not deleted by the test. Since this is a core test, the contrib plugin cannot change the base class for the core test to add a tearDown step where it could do its cleanup. And since this is not done in the DB or files, the simpletest runner does not know it has to clean this.

A hook in the support module for the plugin could perform such cleanup. Of course, for this to be of use, the hook would need to pass information about the test just ran, not only its statistics. A simple way could be to pass $test to hook_test_finished, not just $test->results.

More realistically, a way to wrap test executions (event dispatching ?) could allow such behaviors.

These hooks are only invoked by the UI test runner. run-tests.sh does not invoke them.

I'd venture that we should deprecate these hooks.

I'd venture that we should deprecate these hooks.

Yeah totally. They are broken as you reported, and really, a testing system should not require a working instance of the system itself.

Similar issue: #2460521: Deprecate hook_simpletest_alter()

Rescoping and changing to a task because we're calling these hooks inconsistently. Ordinarily I'd call that a bug, but this is an API that we want to change.

Targeting core 8.4.x because this is disruptive, though we might get it in for 8.3.x if it's deemed reasonable for a testing change.

This patch just marks all the hook_test_*() hooks as deprecated in simpletest.api.php.

The suggestion is:

 * @deprecated in Drupal 8.4.x and will be removed before Drupal 9.0.0. Convert
 *   your test to a PHPUnit-based one and implement test listeners.

This may not be correct for all use-cases, so better suggestions appreciated.

+++ b/core/modules/simpletest/simpletest.api.php
@@ -29,6 +29,12 @@ function hook_simpletest_alter(&$groups) {
+ * This hook is only invoked by the Simpletest UI builder. It will not be
+ * invoked by run-tests.sh or the phpunit tool.
+ *
+ * @deprecated in Drupal 8.4.x and will be removed before Drupal 9.0.0. Convert
+ *   your test to a PHPUnit-based one and implement test listeners.

@@ -37,6 +43,12 @@ function hook_test_group_started() {
+ * This hook is only invoked by the Simpletest UI builder. It will not be
+ * invoked by run-tests.sh or the phpunit tool.
+ *
+ * @deprecated in Drupal 8.4.x and will be removed before Drupal 9.0.0. Convert
+ *   your test to a PHPUnit-based one and implement test listeners.
  */

@@ -46,11 +58,17 @@ function hook_test_group_finished() {
  *
+ * This hook is only invoked by the Simpletest UI builder. It will not be
+ * invoked by run-tests.sh or the phpunit tool.
+ *
...
+ *
+ * @deprecated in Drupal 8.4.x and will be removed before Drupal 9.0.0. Convert
+ *   your test to a PHPUnit-based one and implement test listeners.
  */

I wonder whether we can throw e_trigger_error somehow in case those hooks are implemented.

The CI error looks like a broken testbot. Re-testing.

We could throw an error where the hook is invoked, in the follow-up.

Oops, I misspoke. The follow-up is for D9 which is too late. :-)

This patch adds a function to system module which throws errors based on whether any modules implement the given hook.

This is known as 'scope creep.' :-)

Nice! I'm wondering whether this could be also a static method on the ModuleHandler?

Nice! I'm wondering whether this could be also a static method on the ModuleHandler?

This really needs to be another issue. And here it is: #2866779: Add a way to trigger_error() for deprecated hooks

Looks good, just some nitpicks:

1. +++ b/core/modules/system/system.module
   @@ -1489,3 +1489,31 @@ function system_query_entity_reference_alter(AlterableInterface $query) {
   +
   

   Surplus blank line here.

2. +++ b/core/modules/system/system.module
   @@ -1489,3 +1489,31 @@ function system_query_entity_reference_alter(AlterableInterface $query) {
   + * Helper function to trigger errors for deprecated hooks.
   

   Maybe put a @todo on this, saying to should get replaced as part of work on https://www.drupal.org/node/2866779 ?

Let's postpone on #2866779: Add a way to trigger_error() for deprecated hooks

This patch removes all the trigger_error() stuff, leaving only deprecation notices and @todos about signaling the hooks as deprecated during invocation after #2866779: Add a way to trigger_error() for deprecated hooks

Also #2866779: Add a way to trigger_error() for deprecated hooks is postponed until the testing infrastructure is sorted in #2488860: Bring phpunit bridge into drupal and use it for unit tests and simpletest to handle Deprecation, so unpostponing here.

Planning here: #2870058: [policy, no patch] Document how to deprecate hooks

Postponed on #2866779: Add a way to trigger_error() for deprecated hooks

#2866779: Add a way to trigger_error() for deprecated hooks is in, so we're unpostponed here.

#2866779: Add a way to trigger_error() for deprecated hooks was the follow-up so removing that tag.

This adds handy-dandy deprecation errors via that issue.

I ran into some failing tests locally, because of our old friend SimpleTestBrowserTest. Let's see how different the testbot environment is to mine.

Added a change record: https://www.drupal.org/node/2934242

1. +++ b/core/modules/simpletest/simpletest.api.php
   @@ -29,6 +29,12 @@ function hook_simpletest_alter(&$groups) {
   + * @deprecated in Drupal 8.5.x and will be removed before Drupal 9.0.0. Convert
   
   @@ -37,6 +43,12 @@ function hook_test_group_started() {
   + * @deprecated in Drupal 8.5.x and will be removed before Drupal 9.0.0. Convert
   
   @@ -46,11 +58,17 @@ function hook_test_group_finished() {
   + * @deprecated in Drupal 8.5.x and will be removed before Drupal 9.0.0. Convert
   

   Let's use 8.6.x for now.

2. +++ b/core/modules/simpletest/simpletest.api.php
   @@ -46,11 +58,17 @@ function hook_test_group_finished() {
     *
   + * This hook is only invoked by the Simpletest UI form runner. It will not be
   + * invoked by run-tests.sh or the phpunit tool.
   + *
   
   +++ b/core/modules/simpletest/simpletest.module
   @@ -161,7 +161,7 @@ function simpletest_run_tests($test_list) {
   +  \Drupal::moduleHandler()->invokeAllDeprecated('Convert your test to a PHPUnit-based one and implement test listeners.', 'test_group_started');
   
   @@ -428,7 +428,7 @@ function _simpletest_batch_operation($test_list_init, $test_id, &$context) {
   +    \Drupal::moduleHandler()->invokeAllDeprecated('Convert your test to a PHPUnit-based one and implement test listeners.', 'test_finished', [$test->results]);
   
   @@ -490,7 +490,7 @@ function _simpletest_batch_finished($success, $results, $operations, $elapsed) {
   +  \Drupal::moduleHandler()->invokeAllDeprecated('Convert your test to a PHPUnit-based one and implement test listeners.', 'test_group_finished');
   

   Can we point to the change record everywhere, please?

#2460521: Deprecate hook_simpletest_alter() adds a test which implements hook_simpletest_alter() in a test module. We could use that test module here for the test we need to add, so this is kind of soft-postponed on that.

Fixes #30.

Still to do: Figure out how to run simpletest UI's batch runner without running any tests, so we can invoke the deprecated hooks and prove that they trigger errors. _simpletest_batch_finished() is pretty easy to fake out, and that allows us to prove hook_test_group_finished(), but the others are more tricky.

Mocks and more mocks. Demonstrating why we should use \Drupal::database() (see: #2991337: Document the recommended ways to obtain the database connection object).

Now we have demonstrations that the deprecation errors are thrown.

Updated CR for 8.7.x.

Wrong function signature.

Reroll.

LGTM.

+++ b/core/modules/simpletest/tests/modules/simpletest_deprecation_test/simpletest_deprecation_test.module
--- /dev/null
+++ b/core/modules/simpletest/tests/modules/simpletest_deprecation_test/src/Tests/InnocuousTest.php

+++ b/core/modules/simpletest/tests/modules/simpletest_deprecation_test/src/Tests/InnocuousTest.php
+++ b/core/modules/simpletest/tests/modules/simpletest_deprecation_test/src/Tests/InnocuousTest.php
@@ -0,0 +1,38 @@

@@ -0,0 +1,38 @@
+<?php
+
+namespace Drupal\simpletest_deprecation_test\Tests;
+
+use Drupal\simpletest\WebTestBase;
+
+/**
+ * A very simple WebTestBase test that never touches the database.
+ *
+ * @group simpletest_deprecation_test
+ */
+class InnocuousTest extends WebTestBase {
+
+  /**
+   * Override to prevent any environmental side-effects.
+   */
+  protected function prepareEnvironment() {
+  }
+
+  /**
+   * Override run() since it uses TestBase.
+   */
+  public function run(array $methods = []) {
+  }
+
+  /**
+   * Override to prevent any assertions from being stored.
+   */
+  protected function storeAssertion(array $assertion) {
+  }
+
+  /**
+   * Override to prevent any assertions from being stored.
+   */
+  public static function insertAssert($test_id, $test_class, $status, $message = '', $group = 'Other', array $caller = array()) {
+  }
+
+}

Let's move this into the same file as TestDeprecatedTestHooks. There's no need to make the WebTestBase counter go up by one...
00:09:24.965 Drupal\simpletest_deprecation_test\Tests\InnocuousTest 0 passes from the console output.

Plus let's changes it's @group annotation to be

 * @group WebTestBase
 * @group legacy

To be inline with other remaining WTB - even though this is now very fake.

This looks great overall. I like the fact that this issue triggered deprecating hooks in the first place.
Reading the test this all seems sensible, just a small comment..

+++ b/core/modules/simpletest/simpletest.module
@@ -138,6 +138,8 @@ function simpletest_run_tests($test_list) {
+  // Use the database service so that we can inject a database for testing this
+  // function.

This is an unnecessary change, also I don't understand what information this comment adds :)

Committed dad18d8 and pushed to 8.7.x. Thanks!

Crediting @dawehner, @alberto56 and @joachim for review comments.

diff --git a/core/modules/simpletest/simpletest.module b/core/modules/simpletest/simpletest.module
index cf35b7f98e..1d0e7b2cb7 100644
--- a/core/modules/simpletest/simpletest.module
+++ b/core/modules/simpletest/simpletest.module
@@ -138,8 +138,6 @@ function simpletest_run_tests($test_list) {
     unset($test_list['phpunit']);
   }
 
-  // Use the database service so that we can inject a database for testing this
-  // function.
   $test_id = \Drupal::database()->insert('simpletest_test_id')
     ->useDefaults(['test_id'])
     ->execute();

Removed comment on commit as per #41.

Sadly this broke PHP 5.5 - https://www.drupal.org/pift-ci-job/1182207

Looking into the problem deeper I think the function signature is wrong. We never generate a SPLString, we just have it in core.services.yml as we didn't have a better way to generate a non object service.
In general Drupal core does not require the spltypes php extension so we should not use them in our code.
Nothing in Drupal core passes along a different site path which is its own problem.

I'd RTBC but I wrote the earlier patch. Starting tests up.

I doubt we're supporting PHP 5 at all in core 8.8.x, but this still solves that problem.

Fixed a test so it's not risky.

Still needs updated deprecation messages per our CS.

Updated @deprecation annotation. Further updated test to not be risky.

+++ b/core/modules/simpletest/simpletest.api.php
@@ -53,11 +69,19 @@ function hook_test_group_finished() {
  * @see \Drupal\simpletest\WebTestBase::results()
+ *
+ * @deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. Convert your
+ *   test to a PHPUnit-based one and implement test listeners.
+ *
+ * @see https://www.drupal.org/node/2934242

Those additions have to be added above the @see \Drupal\simpletest\WebTestBase::results() line. That will makes @see annotations grouped.

Regarding other changes +1 for rtbc

Thanks, @voleger.

Addresses #54.

Issues that caused this to be reverted and additional feedback have been addressed, so this looks ready.

Second time lucky. The whole \SplString service thing is currently deprecated in Symfony 4.4 so it's likely this will change in Drupal 9 anyway and the removal of a typehint is not BC breaking (especially when the typehint is wrong).

Committed 0125272 and pushed to 8.8.x. Thanks!

Re #57: Actually there is a warning with that fix:

Warning : Declaration of Drupal\filebrowser\FilebrowserStream\FilebrowserStream::basePath(?SplString $site_path = NULL) should be compatible with Drupal\Core\StreamWrapper\PublicStream::basePath($site_path = NULL) in require()

I don't know if it worth creating a new issue.