Refactor simpletest's *_phpunit_*() (and junit) functions etc. to a class, deprecate

Rather than spending time here it would be better IMHO to get rid of the simpletest <-> phpunit integration and rather provide a phpunit wrapper script which fullfills the purposes of the testbot.

@Mile23
Do you understand my point aka. the time invested into rewriting those bits is less effective than getting rid of it in the first place? No question though in general, the new code is much better, especially better tested.

Thanks a ton for trying to improve those things! A ton!

However, we have it and it sucks.

Well, what we actually just need is a small wrapper script for phpunit which does the handling of the fatals. https://github.com/brianium/paratest kinda provides that in some way already.

I'll review your work later, no worries.

Just a couple of formatting nitpicks:

1. +++ b/core/lib/Drupal/Core/Test/JUnitConverter.php
   @@ -0,0 +1,122 @@
   +/**
   + * @file
   + * Contains \Drupal\simpletest\JUnitConverter.
   + */
   

   @file docblocks are deprecated for classes.

2. +++ b/core/tests/Drupal/Tests/Core/Test/PhpUnitTestRunnerTest.php
   @@ -0,0 +1,71 @@
   +    $log_path = 'test_log_path';
   +   ¶
   +    // Create a mock runner.
   

   Stray whitespace.

I really like separating the concerns here. It makes things much easier to follow.

1. +++ b/core/lib/Drupal/Core/Test/JUnitConverter.php
   @@ -0,0 +1,122 @@
   + */
   +class JUnitConverter {
   

   Conceptually I'm wondering whether its better to make those methods non static, especially when you think about a converter as some actual existing thing you can initialize.

2. +++ b/core/modules/simpletest/simpletest.module
   @@ -283,9 +274,13 @@ function simpletest_phpunit_xml_filepath($test_id) {
     */
    function simpletest_phpunit_configuration_filepath() {
   

   Do you mind open a follow up to discuss dropping this function? This is not actually used anywhere in core and is actually conceptually wrong :) We should pick of core/phpunit.xml as well

3. +++ b/core/modules/simpletest/simpletest.module
   @@ -301,62 +296,14 @@ function simpletest_phpunit_configuration_filepath() {
   - *   The results as returned by exec().
   + *  The results as returned by exec().
   

   Let's keep the additional space :)

4. +++ b/core/modules/simpletest/tests/src/Unit/SimpletestPhpunitRunCommandTest.php
   @@ -3,35 +3,79 @@
   +  protected $fixture_container;
   

   Nitpick: Is there a reason we use snake_case :)?

A little nitpicking, but other than that looks good to me.

+/**
+ * @file
+ * Contains \Drupal\simpletest\JUnitConverter.
+ */

These are no longer used.

> + * Finds all test cases recursively from a test suite list.

In passing -- some of the docs here aren't very clear. But best to keep this a clean refactor and improve docs later.

  public static function create(ContainerInterface $c) {

All container-aware classes I've seen so far call it $container. For DX I think that should be kept consistent.

This makes certainly the code better to follow. While I personally think focusing on converting away from simpletest would be a better invest of time, and experiment with paratest for example, but for sure, this patch, as it is, is a nice thing!

1. +++ b/core/lib/Drupal/Core/Test/JUnitConverter.php
   @@ -0,0 +1,117 @@
   + * This is mainly for converting PHPUnit output.
   + */
   +class JUnitConverter {
   
   +++ b/core/lib/Drupal/Core/Test/PhpUnitTestRunner.php
   @@ -0,0 +1,234 @@
   + * $results = $runner->runTests($test_id, $test_list['phpunit']);
   + * @endcode
   + */
   +class PhpUnitTestRunner {
   

   Can we mark those classes explicit as internal? IMHO they are not meant to be used outside of core, or are they?

2. +++ b/core/modules/simpletest/simpletest.module
   @@ -178,29 +181,13 @@ function simpletest_run_tests($test_list) {
   + *   Drupal\Core\Test\PhpUnitTestRunner::runTests().
   
   @@ -273,9 +260,13 @@ function simpletest_summarize_phpunit_result($results) {
   + *   Drupal\Core\Test\PhpUnitTestRunner::xmlLogFilepath().
   
   @@ -283,9 +274,13 @@ function simpletest_phpunit_xml_filepath($test_id) {
   + *   Drupal\Core\Test\PhpUnitTestRunner::phpUnitConfigurationFilepath().
   
   @@ -302,61 +297,13 @@ function simpletest_phpunit_configuration_filepath() {
   + *   Drupal\Core\Test\PhpUnitTestRunner::runCommand().
   
   @@ -364,25 +311,13 @@ function simpletest_phpunit_run_command(array $unescaped_test_classnames, $phpun
   + *   Drupal\Core\Test\PhpUnitTestRunner::phpUnitCommand().
   
   @@ -752,18 +687,12 @@ function simpletest_mail_alter(&$message) {
   + *   Drupal\Core\Test\JUnitConverter::xmlToRows().
   
   @@ -776,34 +705,12 @@ function simpletest_phpunit_xml_to_rows($test_id, $phpunit_xml_file) {
   + *   Drupal\Core\Test\JUnitConverter::findTestCases().
   
   @@ -816,32 +723,10 @@ function simpletest_phpunit_find_testcases(\SimpleXMLElement $element, \SimpleXM
   + *   Drupal\Core\Test\JUnitConverter::testcaseToSimpletestRow().
   
   +++ b/core/modules/simpletest/tests/src/Unit/SimpletestPhpunitRunCommandTest.php
   @@ -3,35 +3,79 @@
   + * Drupal\Tests\simpletest\Unit\SimpletestPhpunitRunCommandTestWillDie which can
   
   +++ b/core/tests/Drupal/Tests/Core/Test/PhpUnitTestRunnerTest.php
   @@ -0,0 +1,66 @@
   + * @see Drupal\Tests\simpletest\Unit\SimpletestPhpunitRunCommandTest
   

   Nitpick, let's start with "\" if possible :)

> Can we mark those classes explicit as internal? IMHO they are not meant to be used outside of core, or are they?

If you mean with an @internal tag, then that's not actually yet part of our standard!

@joachim
Well, I'm more of a fan of structuring our standards around the idea what people actually use vs. the other way round.

lib/Drupal/Component/Render/MarkupInterface.php:14: * filtering itself, then it must be marked as "@internal". For example, Views
lib/Drupal/Core/Entity/Routing/AdminHtmlRouteProvider.php:15: * @internal
lib/Drupal/Core/Entity/Routing/DefaultHtmlRouteProvider.php:30: * @internal
lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php:256:   * @internal Only to be used internally by Entity API. Expected to be
lib/Drupal/Core/Field/FieldFilteredMarkup.php:16: * @internal
lib/Drupal/Core/Render/MainContent/HtmlRenderer.php:287:   * @internal
lib/Drupal/Core/Render/MainContent/HtmlRenderer.php:324:   * @internal
lib/Drupal/Core/Render/Markup.php:15: * @internal
lib/Drupal/Core/Render/RenderCacheInterface.php:8: * @internal
lib/Drupal/Core/Render/RenderCache.php:14: * @internal
lib/Drupal/Core/Routing/UrlGeneratorInterface.php:76:   * @internal
lib/Drupal/Core/Theme/Registry.php:16: * @internal
lib/Drupal/Core/TypedData/Validation/ExecutionContext.php:120:   * @internal Called by \Drupal\Core\TypedData\Validation\ExecutionContextFactory.
lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php:25: * information about @internal.
lib/Drupal/Core/Utility/LinkGeneratorInterface.php:71:   * @internal
lib/Drupal/Core/Utility/LinkGeneratorInterface.php:86:   * @internal
modules/aggregator/aggregator.module:169: * @internal
modules/big_pipe/src/Render/BigPipeMarkup.php:15: * @internal
modules/content_moderation/src/Entity/ContentModerationState.php:131:   * @internal
modules/content_translation/content_translation.admin.inc:155: * @internal
modules/filter/src/Render/FilteredMarkup.php:15: * @internal
modules/migrate/src/Plugin/Discovery/ProviderFilterDecorator.php:11: * @internal
modules/migrate/src/Plugin/Discovery/StaticReflectionParser.php:10: * @internal
modules/migrate/src/Plugin/Discovery/AnnotatedClassDiscoveryAutomatedProviders.php:15: * @internal
modules/rest/src/Plugin/rest/resource/EntityResourceAccessTrait.php:9: * @internal
modules/rest/src/Plugin/rest/resource/EntityResourceValidationTrait.php:10: * @internal
modules/system/src/Form/PrepareModulesEntityUninstallForm.php:204:   * @internal
modules/views/src/Render/ViewsRenderPipelineMarkup.php:15: * @internal
tests/Drupal/KernelTests/KernelTestBase.php:243:   * @internal
tests/Drupal/KernelTests/KernelTestBase.php:444:   * @internal
themes/bartik/bartik.info.yml:1:# This theme is marked as @internal. It is intended to evolve and change over
themes/seven/seven.info.yml:1:# This theme is marked as @internal. It is intended to evolve and change over

The concept of @internal kinda exists already.

Indeed, but because it's not been set in our standards, API module doesn't support it yet...

Anyway, the boat's sailed on this, so I don't think adding more instances of this is a problem.

@joachim
Thank you for that :)

So yah, @internal for both classes.

Cool, yeah there are areas which really simply shouldn't be part of an external API, but still, of course well documented.
Do you understand the three failures?

1. +++ b/core/lib/Drupal/Core/Test/PhpUnitTestRunner.php
   @@ -0,0 +1,251 @@
   +   *   (optional) The exit status code of the PHPUnit process will be assigned to
   ...
   +    // Setup an environment variable containing the base URL, if it is available.
   

   Ubernitpick: 80 chars line, but annoying for commiters to fix it :(

2. +++ b/core/lib/Drupal/Core/Test/PhpUnitTestRunner.php
   @@ -0,0 +1,251 @@
   +    // A $status of 0 = passed test, 1 = failed test, > 1 indicates segfault
   +    // timeout, or other type of failure.
   +    if ($status > 1) {
   +      // Something broke during the execution of phpunit.
   +      // Return an error record of all failed classes.
   +      $rows[] = [
   +        'test_id' => $test_id,
   +        'test_class' => implode(",", $unescaped_test_classnames),
   +        'status' => 'fail',
   +        'message' => 'PHPunit Test failed to complete; Error: ' . implode("\n", $output),
   +        'message_group' => 'Other',
   +        'function' => implode(",", $unescaped_test_classnames),
   +        'line' => '0',
   +        'file' => $phpunit_file,
   +      ];
   +    }
   +
   +    if ($status === 1) {
   +      $rows[] = [
   +        'test_id' => $test_id,
   +        'test_class' => implode(",", $unescaped_test_classnames),
   +        'status' => 'fail',
   +        'message' => 'PHPunit Test failed to complete; Error: ' . implode("\n", $output),
   +        'message_group' => 'Other',
   +        'function' => implode(",", $unescaped_test_classnames),
   +        'line' => '0',
   +        'file' => $phpunit_file,
   +      ];
   +    }
   

   Good observation. Well open an issue and carry on :) We solved an issue by introducing a smaller one

Anything left to be done here? The cleanup/conversions look really great. +1 for RTBC here.

This makes sense, the biggest problem I have with the patch is that it does not actually remove usages of the now deprecated simpletest_*() functions. I think we should do it in this issue right away, but we can also do it as follow-up.

Some random nitpicks:

1. +++ b/core/lib/Drupal/Core/Test/PhpUnitTestRunner.php
   @@ -0,0 +1,250 @@
   +   * @param ContainerInterface $container
   

   should use the full qualified class name.

2. +++ b/core/lib/Drupal/Core/Test/PhpUnitTestRunner.php
   @@ -0,0 +1,250 @@
   +   * @param array $unescaped_test_classnames
   +   *   An array of test class names, including full namespaces, to be passed as
   +   *   a regular expression to PHPUnit's --filter option.
   

   data type should be "string[]", right?

3. +++ b/core/modules/simpletest/tests/src/Unit/SimpletestPhpunitRunCommandTest.php
   @@ -3,35 +3,79 @@
   +   * @var Drupal\Core\DependencyInjection\ContainerBuilder
   

   leading "\" missing

4. +++ b/core/modules/simpletest/tests/src/Unit/SimpletestPhpunitRunCommandTest.php
   @@ -3,35 +3,79 @@
   +   * @coversNothing
   

   we don't really use this annotation and of course your test covers some code? I would remove it.

ok, that works for me.

The number of assertions above matches what the branch results in 8.3.x returns, so our test system still works as it should.

Looks good!

These changes look sensible for 8.x whilst we have to maintain Simpletest. Just one thing...

+++ b/core/lib/Drupal/Core/Test/PhpUnitTestRunner.php
@@ -0,0 +1,234 @@
+  public static function create(ContainerInterface $container) {
+    return new static(
+      (string) $container->get('app.root'),
+      (string) $container->get('file_system')->realpath('public://simpletest')
+    );
+  }

+++ b/core/modules/simpletest/simpletest.module
@@ -304,61 +300,13 @@ function simpletest_phpunit_configuration_filepath() {
+  $runner = PhpUnitTestRunner::create(\Drupal::getContainer());

@@ -366,25 +314,13 @@ function simpletest_phpunit_run_command(array $unescaped_test_classnames, $phpun
+  $runner = PhpUnitTestRunner::create(\Drupal::getContainer());

+++ b/core/modules/simpletest/tests/src/Unit/SimpletestPhpunitRunCommandTest.php
@@ -83,8 +103,17 @@ public function provideStatusCodes() {
+    $runner = PhpUnitTestRunner::create($this->fixtureContainer);

Why not just simpletest.phpunit_runner to simpletest.services and use \Drupal in the procedural code?

Agreed, turning this into a service can be done later. The constructors of the classes seem to be sufficiently decoupled from the container with the create() method. So there should not be API changes when we turn this into a service, where ever it might live.

Patch does not apply anymore.

+++ b/core/lib/Drupal/Core/Test/PhpUnitTestRunner.php
@@ -0,0 +1,237 @@
+    // Setup an environment variable containing the base URL, if it is available.

Line exceeding 80 characters.

Fixed and attached new patch.

Looks good, back to RTBC.

Testbot hiccup

Here's the error reported by Drupal CI for the patch in #78:

Fatal error: Uncaught TypeError: Argument 1 passed to PHPUnit_Framework_TestCase::run() must be an instance of PHPUnit_Framework_TestResult, array given, called in /var/www/html/core/scripts/run-tests.sh on line 793 and defined in /var/www/html/vendor/phpunit/phpunit/src/Framework/TestCase.php:609

Thanks for resolving that, back to RTBC

Nice patch. Just some nits so far.

1. +++ b/core/lib/Drupal/Core/Test/JUnitConverter.php
   @@ -0,0 +1,117 @@
   +   * @return array[]
   +   *   The results as array of rows in a format that can be inserted into
   +   *   {simpletest}.
   

   Can we be a bit more specific? Edit: I mean about the "in a format". Usually we actually want to document what the actual keys are in an associative array. Since these are moved lines, followup would be best.

2. +++ b/core/lib/Drupal/Core/Test/JUnitConverter.php
   @@ -0,0 +1,117 @@
   +   *   (Optional) The parent of the current element. Defaults to NULL.
   

   "Optional" should not be capitalized here. Does this exist in HEAD or?

3. +++ b/core/lib/Drupal/Core/Test/JUnitConverter.php
   @@ -0,0 +1,117 @@
   +   * @return array
   +   *   A list of all test cases.
   

   Can has followup for "array of what".

4. +++ b/core/modules/simpletest/simpletest.module
   @@ -118,8 +116,9 @@ function _simpletest_format_summary_line($summary) {
   - * @param $test_list
   - *   List of tests to run.
   + * @param array[] $test_list
   + *   List of tests to run. The top level is keyed by type of test, either
   + *   'simpletest' or 'phpunit'. Under that is an array of class names to run.
   

   Sounds like it's a string[][]? But actually we probably shouldn't be changing this hunk at all here; presumably the only changes should be gutting and deprecating the function, rather than improving its docs.

5. +++ b/core/modules/simpletest/simpletest.module
   @@ -181,28 +180,13 @@ function simpletest_run_tests($test_list) {
   + *   Drupal\Core\Test\PhpUnitTestRunner::runTests().
   
   @@ -275,9 +259,13 @@ function simpletest_summarize_phpunit_result($results) {
   + *   Drupal\Core\Test\PhpUnitTestRunner::xmlLogFilepath().
   
   @@ -313,65 +301,13 @@ function simpletest_phpunit_configuration_filepath() {
   + *   Drupal\Core\Test\PhpUnitTestRunner::runCommand().
   
   +++ b/core/modules/simpletest/tests/src/Unit/SimpletestPhpunitRunCommandTest.php
   @@ -12,6 +13,14 @@
   + * Drupal\Tests\simpletest\Unit\SimpletestPhpunitRunCommandTestWillDie which can
   

   Minor: I think this needs a leading \ so's not to be Drupal\thing\Drupal\thing. (In these hunks and elsewhere in the patch.)

6. +++ b/core/modules/simpletest/tests/src/Unit/SimpletestPhpunitRunCommandTest.php
   @@ -12,6 +13,14 @@
   + * Here, we run SimpletestPhpunitRunCommandTestWillDie, make it die, and see
   + * what happens.
   

   Make it die! ☠️

We now have a followup for those docs improvements, so setting back to rtbc.

+++ b/core/lib/Drupal/Core/Test/PhpUnitTestRunner.php
@@ -0,0 +1,238 @@
+      putenv('BROWSERTEST_OUTPUT_DIRECTORY=' . \Drupal::service('file_system')->realpath('public://simpletest'));

I think \Drupal::service::... can be replaced with $this->workingDirectory

Also I deliberated findTestcases vs findTestCases and xmlLogFilepath vs xmlLogFilePath currently in core TestCases and FilePath are way more prevalent but the current patch is converting the snake case to camel case so the new names are in line with the old names. Not sure - but I think now might be a good time to bring the names into line with the rest of core. Unfortunately this also means replacing things like $testcases with $test_cases - however this is also totally in line with PHPUnit too.

Looking good.

Just a couple of nits:

1. +++ b/core/lib/Drupal/Core/Test/JUnitConverter.php
   @@ -0,0 +1,117 @@
   +   * @return array[]|null
   

   Why do we need to support NULL? Why not just an empty array?

2. +++ b/core/lib/Drupal/Core/Test/PhpUnitTestRunner.php
   @@ -0,0 +1,237 @@
   +class PhpUnitTestRunner {
   

   Should this implement \Drupal\Core\DependencyInjection\ContainerInjectionInterface ?

3. +++ b/core/modules/simpletest/simpletest.module
   @@ -187,28 +187,16 @@ function simpletest_run_tests($test_list) {
   +  @trigger_error(__FUNCTION__ . ' is deprecated. Use \Drupal\Core\Test\PhpUnitTestRunner::runTests() instead. https://www.drupal.org/node/2948547', E_USER_DEPRECATED);
   
   @@ -281,9 +269,16 @@ function simpletest_summarize_phpunit_result($results) {
   +  @trigger_error(__FUNCTION__ . ' is deprecated. Use \Drupal\Core\Test\PhpUnitTestRunner::xmlLogFilepath() instead. https://www.drupal.org/node/2948547', E_USER_DEPRECATED);
   
   @@ -319,65 +314,16 @@ function simpletest_phpunit_configuration_filepath() {
   +  @trigger_error(__FUNCTION__ . ' is deprecated. Use \Drupal\Core\Test\PhpUnitTestRunner::runCommand() instead. https://www.drupal.org/node/2948547', E_USER_DEPRECATED);
   
   @@ -385,24 +331,16 @@ function simpletest_phpunit_run_command(array $unescaped_test_classnames, $phpun
   +  @trigger_error(__FUNCTION__ . ' is deprecated. Use \Drupal\Core\Test\PhpUnitTestRunner::phpUnitCommand() instead. https://www.drupal.org/node/2948547', E_USER_DEPRECATED);
   
   @@ -842,18 +781,15 @@ function simpletest_mail_alter(&$message) {
   +  @trigger_error(__FUNCTION__ . ' is deprecated. Use \Drupal\Core\Test\JUnitConverter::xmlToRows() instead. https://www.drupal.org/node/2948547', E_USER_DEPRECATED);
   
   @@ -866,34 +802,15 @@ function simpletest_phpunit_xml_to_rows($test_id, $phpunit_xml_file) {
   +  @trigger_error(__FUNCTION__ . ' is deprecated. Use \Drupal\Core\Test\JUnitConverter::findTestCases() instead. https://www.drupal.org/node/2948547', E_USER_DEPRECATED);
   
   @@ -901,40 +818,18 @@ function simpletest_phpunit_find_testcases(\SimpleXMLElement $element, \SimpleXM
   +  @trigger_error(__FUNCTION__ . ' is deprecated. Use \Drupal\Core\Test\JUnitConverter::convertTestCaseToSimpletestRow() instead. https://www.drupal.org/node/2948547', E_USER_DEPRECATED);
   

   We should be using the standard deprecation format.

LGTM!

1. +++ b/core/lib/Drupal/Core/Test/JUnitConverter.php
   @@ -0,0 +1,117 @@
   +      if (!$element->attributes()->class) {
   +        $name = explode('::', $parent->attributes()->name, 2);
   +        $element->addAttribute('class', $name[0]);
   +      }
   +      $test_cases[] = $element;
   +    }
   +    else {
   

   we can return here and avoid the else

2. +++ b/core/lib/Drupal/Core/Test/JUnitConverter.php
   @@ -0,0 +1,117 @@
   +      // @todo: Check on the proper values for this.
   +      'message_group' => 'Other',
   

   do we have an issue for this? is this still an open item for this patch?

3. +++ b/core/lib/Drupal/Core/Test/PhpUnitTestRunner.php
   @@ -0,0 +1,237 @@
   +    return $this->appRoot . '/core/phpunit.xml.dist';
   

   should we also test for the phpunit.xml file which should take precedence? or is that an existing issue and not changed here?

4. +++ b/core/lib/Drupal/Core/Test/PhpUnitTestRunner.php
   @@ -0,0 +1,237 @@
   +    if ($status == TestStatus::PASS) {
   +      $rows = JUnitConverter::xmlToRows($test_id, $phpunit_file);
   +    }
   +    else {
   +      $rows[] = [
   

   same here re early return.

A micro-nit:

+++ b/core/lib/Drupal/Core/Test/PhpUnitTestRunner.php
@@ -0,0 +1,278 @@
+  /**
+   * Factory method to inject the container-based dependencies.
+   *
+   * @param \Symfony\Component\DependencyInjection\ContainerInterface $container
+   *   Container to inject.
+   *
+   * @return static
+   *   Newly-created test runner object.
+   */
+  public static function create(ContainerInterface $container) {

This can just be @inheritdoc now.

Ran some tests using the Simpletest UI and using run-tests.sh with this patch applied and the results are identical with and without the patch (in my small sample anyway).

Looks like we have some phpcs regressions here

Checking changed files...
PHPCS: core/lib/Drupal/Core/Test/JUnitConverter.php passed

FILE: .../git/core/drupal/core/lib/Drupal/Core/Test/PhpUnitTestRunner.php
----------------------------------------------------------------------
FOUND 5 ERRORS AFFECTING 5 LINES
----------------------------------------------------------------------
 224 | ERROR | [x] Closing parenthesis of array declaration must be
     |       |     on a new line
 253 | ERROR | [x] There must not be a single space before a unary
     |       |     operator statement
 257 | ERROR | [x] There must not be a single space before a unary
     |       |     operator statement
 261 | ERROR | [x] There must not be a single space before a unary
     |       |     operator statement
 265 | ERROR | [x] There must not be a single space before a unary
     |       |     operator statement
----------------------------------------------------------------------
PHPCBF CAN FIX THE 5 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 147ms; Memory: 8MB

PHPCS: core/modules/simpletest/simpletest.module passed
PHPCS: core/modules/simpletest/src/Tests/UiPhpUnitOutputTest.php passed
PHPCS: core/modules/simpletest/tests/src/Kernel/PhpUnitErrorTest.php passed

FILE: ...odules/simpletest/tests/src/Kernel/SimpletestDeprecationTest.php
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
 73 | ERROR | [x] Closing parenthesis of array declaration must be on
    |       |     a new line
 77 | ERROR | [x] Closing parenthesis of array declaration must be on
    |       |     a new line
----------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 180ms; Memory: 8MB

PHPCS: core/modules/simpletest/tests/src/Unit/SimpletestPhpunitRunCommandTest.php passed

FILE: .../drupal/core/tests/Drupal/Tests/Core/Test/JUnitConverterTest.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 76 | ERROR | [x] Closing parenthesis of array declaration must be on
    |       |     a new line
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 149ms; Memory: 8MB


FILE: ...upal/core/tests/Drupal/Tests/Core/Test/PhpUnitTestRunnerTest.php
----------------------------------------------------------------------
FOUND 6 ERRORS AFFECTING 5 LINES
----------------------------------------------------------------------
  91 | ERROR | [x] Closing parenthesis of array declaration must be
     |       |     on a new line
  98 | ERROR | [x] Closing parenthesis of array declaration must be
     |       |     on a new line
 105 | ERROR | [x] Closing parenthesis of array declaration must be
     |       |     on a new line
 112 | ERROR | [x] Closing parenthesis of array declaration must be
     |       |     on a new line
 128 | ERROR | [x] Expected 1 space after FOREACH keyword; 0 found
 128 | ERROR | [x] There should be no white space after an opening
     |       |     "("
----------------------------------------------------------------------
PHPCBF CAN FIX THE 6 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

phpcs seems to get confused by having two [[ together. This patch just splits them out onto new lines.

#116 looks good, feedback has been addressed, back to RTBC.

Committed 76f3729 and pushed to 8.8.x. Thanks!

🎉 published the change record