Add an attribute for skipping PHPUnit tests

Maybe first step would be to file an issue upstream @ https://github.com/sebastianbergmann/phpunit/issues just to see if there's any interest to have such an attribute directly in PHPUnit?

Interesting development upstream, Retry failing tests up to N times using the #[Retry] attribute #6742

#3615036: Replace markTestSkipped() with the Skip attribute inspired me to try doing this because I thought "This can't be difficult to do. If it isn't, then we shouldn't spend time moving all of the markTestSkipped() calls." It turned out to be fast to implement. I converted two skips, one with a message and one without.

The priority of 200 in the new "Before" function was chosen somewhat arbitrarily to run before anything else.

This looks great to me. Since we should never have dozens and dozens of skipped methods in core I don't think we need to be too concerned about trying to get support for this upstream. Would be easy enough to convert later if it happens.

Would be easy enough to convert later if it happens.

That was my thought too. Also, we can use a solution like this for early skips, the sooner the better. Any solution introduced in PHPUnit would be for a version that we may not be able to add as a dependency for months or years. I think it's better to accept that we might have to deprecate our home grown solution later on. In the meantime we can point Sebastian at this and say "Are you interested in implementing this?"

I wrote a change record.

How about just #[Skip]? It’s obvious we are skipping a test here. Would also match with the #[Retry] and #[Repeat] methods coming up with PhpUnit 13.3

Tweaked the CR to Skip but lets do it!

Thank you for doing that! I got pulled into a meeting right as I was wrapping up the changes.

How about just #[Skip]?

This sounded like a good idea to me, so I did it. Thank you.

Confirmed Drupal\Tests\Core\Site\SettingsTest::testRealDeprecatedSettings is skipped as expected: https://git.drupalcode.org/project/drupal/-/jobs/11422791 (need to click to view the raw log)

And Drupal\Tests\editor\Kernel\EditorValidationTest::testLabelValidation skipped: https://git.drupalcode.org/project/drupal/-/jobs/11422789

Had a couple minor suggestions and went ahead and applied them:

• (Optional) -> (optional), lowercase is used consistently through core
• $this->markTestSkipped -> static::markTestSkipped, because it's a static method
• $attribute->getArguments()[0] ?? '' -> $attribute->newInstance()->message), easier to read

Committed f4a46dd and pushed to main. Thanks!

Does not apply to 11.x, so will need a separate MR for that. For now, I made a couple small edits to the CR and set the version to 12.0.0, which will need to be updated after the backport.

One suggestion I have for a future enhancement is to add a callable as a second optional parameter to Skip, and that callable could be invoked (in a static context), and the test would be skipped on whether it returns TRUE.

...because it's a static method

Probably no one ever noticed because the PHPUnit docs use $this->markTestSkipped(). I think this same PHPUnit coding standard was discovered for some other static function during the mock->stub conversion project.

Backported

Judging from #3613344: Remove update_fetch_with_http_fallback and #3591513: Deprecate the update_fetch_with_http_fallback setting, I think SettingsTest::testRealDeprecatedSettings isn't supposed to be skipped in 11.x. I have suggestions on the MR to remove. Could someone confirm that and apply the suggestions if correct? Can go back to RTBC after.

I'm not at my desk but I was involved in that issue and that sounds right, it shouldn't be skipped in 11.x. I guess if it passes when it is unskipped then there is the proof.

Removed the skip per #27 and tests pass, so back to RTBC. Will commit once I back at the computer.

Committed f7eadfb and pushed to 11.x. Thanks!