Replace markTestSkipped() with the Skip attribute

This sounds familiar. Added some related issues.

In #3516771: Prevent setup tasks running for tests that are always skipped we had to skip some methods only, so used a condition like this in setUp:

if ($this->name() === 'testAnnounceFeedUpdatedAndRemoved') {
  $this->markTestSkipped('Skipped due to major version-specific logic. See https://www.drupal.org/project/drupal/issues/3359322');
}

#3517430: Add an attribute for skipping PHPUnit tests is a suggestion that would be better DX.

Hi @catch, @mstrelan

I’ve made the necessary changes, and everything is now ready for review.
Please let me know if you need anything else.

Cheers!
Charllie

I reviewed the MR last night. Several classes that are affected only have a single test function. In those instances, the markTestSkipped() in the setUp() was not wrapped in a condition for the function name. Personally, I think it should be a best practice to always add a condition wrapper. New test methods could be added to these classes at any time. We wouldn't want a situation where no one notices the skip.

RE #3: While I was reviewing this I thought it couldn't be difficult to add the proposed attribute. So I did it in #3517430: Add an attribute for skipping PHPUnit tests. If it's decided that we want to go that direction, then it would be better to repurpose this issue for adding the attribute to methods instead of the current approach. And in that case this should be postponed.

The concept in #3517430: Add an attribute for skipping PHPUnit tests was given the OK and that issue will move forward. I'm postponing this issue on the resolution of the other. Afterward, this issue can have its scope changed to implement the new attribute.

This is unblocked for the main branch. As of the time I'm writing this, the Skip attribute hasn't been backported to 11.x yet.

I'm changing the scope of this issue to implement the new Skip attribute. I've updated the issue summary accordingly.

I'm sorry that the previous work that was done for this issue is no longer relevant, but the new solution will be more sustainable for the long-term. Whoever continues work on this can proceed with editing MR 16573, but it may be easiest to close that and open a new one.

The Skip attribute has been added to 11.x too. So this issue is unblocked on both active branches.

Hi @dcam

I’ve created a new MR for review. If there’s anything else I need to change or do, please let me know.

I found one problem and had one question. I'm also going to contact the other maintainers to see what they think about all the associated @todo comments, if and where they should be moved. Hopefully we'll get a response or two.

Oh, and thank you for putting up with the change in direction of the issue, @charlliequadros. You've done good work.

You're welcome! If you need any other changes, just let me know. :D

Here's what we came up with for the code comments that are associated with the skips we're moving:

• We're OK with deleting the @todo and other comments. The Skip attribute alone should suffice for finding and removing them in the future.
• Skip messages should be standardized.
• If a skip is temporary, change its message to #[Skip('Temporary: see https://www.drupal.org/node/#######')] where the URL is the issue that will remove the skip. The issue should contain all of the details about why the skip was added and why it will be removed.
• If a skip is permanent, change its message to #[Skip('Permanent: see https://www.drupal.org/node/#######')] where the URL is the issue that added the skip.
• The standardized skip message information should be added to our coding practices after this issue has been committed. I've tagged this issue as needing a follow-up for that.

If any of that isn't clear, then let me know.

Contribution credit was given to participants in the Slack maintainer channel conversion.

Hi @dcam

The feedback from comment #19 has been addressed in the updated MR.

The Skip attributes now consistently use either Temporary or Permanent, with links pointing to the corresponding issues. The associated @todo comments were also removed where they are no longer needed.

For temporary skips, the issues tracking removal of the skip are used, including:

#2918718 for MoveBlock
#3542883 for SettingsTray
#3508109 for the three Package Manager tests
#3400150 for Timestamp

For permanent skips, the issues related to the introduction or reason for the skip are used, including #3059332 and #3047812.

DeprecatedAssetsTest now uses xxHash instead of MD5 because Drupal core's PHPStan rules prohibit the use of md5().

Ready for another review.

This looks great. I left one comment because I noticed an outlier among the affected tests. We developed our brand-new skip message standard in response to my concern that we were moving skips without their associated metadata comments. I didn't want people deleting temporary skips someday and leaving those comments behind. But since the method itself is temporary I feel like we don't need a skip message at all.

DeprecatedAssetsTest now uses xxHash instead of MD5 because Drupal core's PHPStan rules prohibit the use of md5().

Right, I just didn't know if PHPStan complained about it or if this was a "while I'm in here working" kind of change. We try to avoid making unrelated edits because it can cause other MRs to need to be rebased more frequently and it can cause confusion for reviewers.

Restore the comments and add skip to the tests in the ViewsConfigUpdaterTest file.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.