#2296615: Accurately support multiple @groups per test class introduces support for multiple @groups in tests, but also introduced a bug where every test is run for every @group it has, instead of just once.

In run-tests.sh we need to account for these multiple groups and we need to make sure every test is only run once.

CommentFileSizeAuthor
#9 2296615-56.patch467 byteslendude
#5 3039572_5.patch5.62 KBmile23
#2 2296615-56.patch467 byteslendude

Comments

Lendude created an issue. See original summary.

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
Status: Active » Needs review
Related issues: +#2296615: Accurately support multiple @groups per test class
StatusFileSize
new 2296615-56.patch467 bytes

This is the patch from #2296615: Accurately support multiple @groups per test class. In run-tests.sh just run the test list through array_unique to make sure tests are not in the list multiple times.

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented

Looking at the test log, #2 doesn't help unfortunately.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@Lendude what are you seeing? The cut off can make it difficult. UpdatePathTestBaseFill aka UpdatePathTestBaseFilledTest is now there only once. Whereas in HEAD it is there twice.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Version: 8.8.x-dev » 8.7.x-dev
StatusFileSize
new 3039572_5.patch5.62 KB

So this is a (not-very-sophisticated) partial solution to #2945465: Make TestDiscovery find info we need, not info we don't need

Refactor TestDiscovery a little to give us the stuff we want.

Status: Needs review » Needs work

The last submitted patch, 5: 3039572_5.patch, failed testing. View results

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@Mile23 if we try for a more complex solution here then I'm just going to revert the original patch. Any chance of an explanation of why the patch in #2 is not working because I can't see why anyone thinks that.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

Using the console logs and limiting to unit tests as a sample (because it's easy to copy/paste)...

#2 doesn't have duplicates.

HEAD does.

Do this by copypasting the list of tests into a text editor, saving, using cut and sort and uniq -c to count everything up. It'd be nice if there was a tool for junit to figure this out, but oh well.

#5 moves towards maintainability. Someone could say 'needs a test' and that could happen. #2 does accomplish the goal, but the way we verify it is with cut, sort, and uniq -c.

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
Issue tags: +Needs manual testing
StatusFileSize
new 2296615-56.patch467 bytes

Drupal\Tests\system\Functional\Entity\Update\LangcodeToAscii is still a dupe in the list....but turns out I randomly picked one that is supposed to be a dupe, duh!!

\Drupal\Tests\system\Functional\Entity\Update\LangcodeToAsciiUpdateTest
\Drupal\Tests\system\Functional\Entity\Update\LangcodeToAsciiUpdateFilledTest

sorry for the confusion, I blame beer!

Reupping the patch in #2, we have an issue to do #5, so I'd also be in favour of not making this more complex then fixing an oversight in the old issue

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
Status: Needs work » Needs review
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Needs review » Reviewed & tested by the community

Lets do this and save cpu cycles and refactor later

alexpott’s picture

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

Committed and pushed be9af5bda6 to 8.8.x and 8d4e3ec389 to 8.7.x. Thanks!

  • alexpott committed be9af5b on 8.8.x 
    Issue #3039572 by Lendude, Mile23: Prevent tests with multiple groups...

  • alexpott committed 8d4e3ec on 8.7.x 
    Issue #3039572 by Lendude, Mile23: Prevent tests with multiple groups...

  • xjm committed c25e035 on 8.7.x 
    Revert "Issue #3039572 by Lendude, Mile23: Prevent tests with multiple...
xjm’s picture

xjm
she/her
Primary language English
commented
Status: Fixed » Reviewed & tested by the community

8.7.x and earlier are still in commit freeze, so this has been reverted from 8.7.x temporarily.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

Not even for a testing improvement that fixes a bug?

  • alexpott committed f6d013b on 8.7.x 
    Issue #3039572 by Lendude, Mile23: Prevent tests with multiple groups...
alexpott’s picture

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

Status: Fixed » Closed (fixed)

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