fix: --parallel does not handle multiple --exclude-group flags (or any other multiple flags)

[flap152]

NOTE: Replaced this PR with a simpler version (#1620) which will hopefully be easier to accept and merge in.

What:

• Bug Fix
• New Feature

Description:

This PR fixes issue 1437 where additional --exlude-group options were ignored when using --parallel flag.

Problem:

In version 3.x and 4.0, running tests with --parallel using multiple --exclude-group options, only the first option was used.
This probably applies to other multiple-use options such as --group.
This problem is made worse in v4.0 using PHPUnit 12, forcing the use of multiple option vs comma-separated list.

Root Cause:

Some plugin argument handlers remove an argument from the $arguments array by using array_flip() twice, wrongly assume the remaining array is unchanged, but any non-unique value is removed from the array.

This breaks Parallel because long options (--option=value) in the argument array appear as two items (['--option','value']) causing potential duplicates.

The non-parallel worker argument pipeline keeps option and value together in the array (['--option=value']) avoiding duplicates.

Solution:

Modified popArgument() in HandleArguments Trait to use unset() instead of double array_flip()
Modified Coverage plugin to use the Trait method instead of its own array_flip() code.

Testing:

Added test case to verify a second --exclude-group is not ignored in parallel mode
Added test case to popArgument() Trait method
Verified existing functionality still works
Test snapshots may need to be updated.

Related:

Fixes issue 1437

[flap152]

Created PR with failing test before pushing solution commits, hoping tests would run and fail.

[yoeriboven]

I'm having the same issue but this doesn't seem to fix it.

None of these exclude the second group.

@php artisan test --parallel --order-by random --exclude-group='browser,architecture'
@php artisan test --parallel --order-by random --exclude-group=browser,architecture
@php artisan test --parallel --order-by random --exclude-group=browser --exclude-group=architecture
@php artisan test --exclude-group=browser --exclude-group=architecture

It does help slightly, if I dump the arguments after the edited call it has browser,architecture where before that it only had browser. It seems to be passed to phpunit correctly (as a comma separated list), but still only excludes the first group.

Tried using testsuites instead of groups and it has the same issue.

It seems like the arguments are passed to php unit correctly, so it's a mystery why it won't work.

[yoeriboven]

Some more info:

I created a new laravel project using phpunit to see if that is where the issue lies. It comes with PHPUnit 11.

When I run ./vendor/bin/phpunit --exclude-group=exclude-one,exclude-two this is the error I get:

1) Using comma-separated values with --exclude-group is deprecated and will no longer work in PHPUnit 12. You can use --exclude-group multiple times instead.

[yoeriboven]

Because I made a stupid debugging mistake I think I got it working before. This works for me with this PR:

php artisan test --parallel --order-by random --exclude-group=browser --exclude-group=architecture

Without this PR it fails.

Thanks @flap152

[flap152]

It won't resolve the failing checks, but I now see a better sequence in using lint/rector/snapshot to make the PR cleaner. I will likely commit tonight.

[flap152]

I returned to using unset() in popArgument() to avoid problems with splice() if the array is not a numeric, zero-based, sequential index.
Also re-indexing of the returned array to avoid problems with calling code to assume as much.
I searched for current usage and nowhere found assumptions on the returned array indexing.

[flap152]

I haven't added a test for it, but this PR also fixes #1478

[Junveloper]

Commenting to follow. Experiencing the same issue.

[flap152]

@nunomaduro I would gladly work on fixing anything wrong in this PR. Eager to learn+help. Thx.

[lindyhopchris]

I think this is why I can't get --parallel working when using --test-directory=value.

[flap152]

Replacing this PR with a simpler version (#1620) which will hopefully be easier to accept and merge in. It does not add tests, therefore avoids breaking Visual tests that use snapshots.