Problem/Motivation

The @group detection fails when you have a class annotation that is indented by more or less than one space. Which is a legimate thing to do, for example when using the namespace something { ... } syntax.

Core doesn't do that, and I guess we don't have coding standards for it, but phpstorm does it by default and I think it makes sense to support that.

Proposed resolution

There doesn't seem to be a reason to hardcode that we need one space in the regex. If we think that this could lease to false detections (I really don't know how?), then we could also do it with a dynamic number of spaces (but nothing else), but that seems to be unnecessarily complicated?

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
#6 simpletest-regex-group-2491915-6-interdiff.txt2.47 KBberdir
#6 simpletest-regex-group-2491915-6.patch2.72 KBberdir
#1 simpletest-regex-group-2491915-1.patch676 bytesberdir
#3 simpletest-regex-group-2491915-3-test-only.patch964 bytesberdir
#3 simpletest-regex-group-2491915-3.patch1.97 KBberdir

Comments

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Active » Needs review
StatusFileSize
new simpletest-regex-group-2491915-1.patch676 bytes
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs review » Needs work
Issue tags: +Needs tests

\Drupal\Tests\simpletest\Unit\TestInfoParsingTest could test this.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review
Issue tags: -Needs tests
StatusFileSize
new simpletest-regex-group-2491915-3.patch1.97 KB
new simpletest-regex-group-2491915-3-test-only.patch964 bytes

I like unit tests that don't require hundred lines of mocking code :)

Also found that I need to update a second check too or the @group ended up in the description.

The last submitted patch, 3: simpletest-regex-group-2491915-3-test-only.patch, failed testing.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Needs review » Needs work

Ran into this issue with monitoring / payment module tests.
https://www.drupal.org/node/2463769#comment-9946987

PHP Fatal error: Uncaught exception 'Drupal\simpletest\Exception\MissingGroupException' with message 'Missing @group annotation in Drupal\Tests\payment_form\Unit\Plugin\Payment\Type\PaymentFormConfigurationFormUnitTest' in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/simpletest/src/TestDiscovery.php:329

+++ b/core/modules/simpletest/src/TestDiscovery.php
@@ -366,7 +366,7 @@ public static function parseTestClassSummary($doc_comment) {
+      if (preg_match('/\*$/', $line) || preg_match('/^ \* \@/', $line)) {

We should consistently check for the starting condition. Here is still a "^ " present that depends on single indentation.

I think we should safely check for the line beginning with any amount of spaces to avoid false detections in doc lines.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review
StatusFileSize
new simpletest-regex-group-2491915-6.patch2.72 KB
new simpletest-regex-group-2491915-6-interdiff.txt2.47 KB

Made the pattern more explicit, some documentation and extend the tests.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

Yeah, now looks good to me.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 684602e and pushed to 8.0.x. Thanks!

  • alexpott committed 684602e on 8.0.x 
    Issue #2491915 by Berdir, miro_dietiker: Test @group detection fails for...

Status: Fixed » Closed (fixed)

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