Deprecate hook_simpletest_alter()

Well, you could really ask yourself wether the hook is the way to do it, given that it requires a proper parent site.

Given that the settings override in settings.testing.php seems great for me.

Note: Also #2304461: KernelTestBaseTNG™ and #2232861: Create BrowserTestBase for web-testing on top of Mink might make this harder than currently.

+++ b/core/modules/simpletest/simpletest.api.php
@@ -13,6 +13,21 @@
+ * This will not from the command line without a settings.php in place
+ * containing a list of test modules:

This sentence doesn't make sense

Thought some more about the intent here - could we have a new annotation on test classes @unsupportedDrivers - which took an array of driver names, defaulting to none?

Then we could tag tests that weren't supported by a given driver.

+++ b/core/lib/Drupal/Core/Test/TestRunnerKernel.php
@@ -51,8 +51,20 @@ public function __construct($environment, $class_loader) {
+      // If there are modules implementing hook_simpletest_alter then
+      // settings.php must exist and supply module data for modules
+      // implementing that hook.

As written in IRC it would be great to explain WHY those module data has to be provided ... this hook is executed outside of an installed drupal site, so you don't have the modules with the alter hook available in the first place.

Thank you!

At the very least we're missing a pointer to this in hook_simpletest_alter.

But for me this approach doesn't feel right. i think we should not use alters for this at all. There should be only one way to achieve this and using hooks means that it'll work when you have a full drupal test runner but then you'll have to know to add this extra stuff to settings.php.

IMO it is valid for a hosting platform to ship with a module that skips certain tests. Similarly, a custom web site might skip Drupal core or contrib tests that it does not care about. Ideally we could specify individual classes to skip because skipping a whole module is reckless overkill Anyway, I just want to articulate use cases beyond DB drivers.

Re-uploading patch for the bots.

I think we should let this later hook just die away. In phpunit you can ignore certain tests using https://stackoverflow.com/questions/22645986/phpunit-testsuite-exclude
so if you want to run the entire core test suite you could ignore the tests by writing your own phpunit.xml file.

Changing to 8.4.x because it's disruptive.

Testbot burp.

Lets postpone on #2866779: Add a way to trigger_error() for deprecated hooks like #2234479: Deprecate hook_test_* hooks in simpletest

And, just like #2234479: Deprecate hook_test_* hooks in simpletest we can unpostpone because #2866779: Add a way to trigger_error() for deprecated hooks is now postponed because core doesn't actually deal with the trigger_error() yet.

Added @todo for #2866779: Add a way to trigger_error() for deprecated hooks

Getting at least this information about is a good step.

Hook deprecation hasn't really been tackled in the deprecation policy but I think we should try and think about how it should be done. I think we need a followup to discuss this. Also there is no change record yet.

Here's a plan for it: #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.

Added change record.

I'm not sure what the follow-up was supposed to be.

Here's a patch against 8.6.x.

+++ b/core/modules/simpletest/simpletest.api.php
@@ -13,10 +13,16 @@
+ * This hook is only invoked by the Simpletest UI builder. It will not be
+ * invoked by run-tests.sh or the phpunit tool.

This isn't true. There are two reasons run-tests.sh needs simpletest module enabled: 1) TestDiscovery depends on ModuleHandler so it can fire this hook and, 2) Functions in simpletest.module. The hook is not fired from the PHPUnit tool so I got that right.

I was thinking of #2234479: Deprecate hook_test_* hooks in simpletest when I wrote that.

@Mile23 Do you understand why this fails?

It has a ton of these: "Drupal\Core\Database\DatabaseNotFoundException: SQLSTATE[HY000] [1049] Unknown database 'jenkins_drupal_patches_46060'"

That has always looked like a testbot failure to me.

Re-running.

It turns out it was a testbot failure.

Fixed the docblock and added a test.

Updated IS.

@Mile23
Do you mind linking to the change record inside the code?

Ah, good catch.

Thank you @Mile23!

The deprecation message should contain a link to the change record. Since this allows us to keep the change record up-to-date with any additional info discovered after committing this.

Updated per #45

Thank you!

Re-rolled after the reversion of #2893117: Improve HTML caching of Simpletest UI test form

Still applies, was already RTBC in #47.

Committed d208812 and pushed to 8.6.x. Thanks!

In reference to #14 - one thing that I think we should try to state somewhere is that core's tests are for core. The idea of a site running them to see if it works is not a good one. There are tests that are designed to be extended like \Drupal\KernelTests\Core\Cache\GenericCacheBackendUnitTestBase but you don't need an alter hook for that.