Problem/Motivation

While working on #3123060: Enable 'Drupal.Commenting.DocComment.ShortSingleLine' coding standard I noticed that \Drupal\Tests\hal\Unit\NormalizerDenormalizeExceptionsUnitTestBase is a data provider for two tests, both in separate files, \Drupal\Tests\hal\Unit\FieldNormalizerDenormalizeExceptionsUnitTest and \Drupal\Tests\hal\Unit\FieldItemNormalizerDenormalizeExceptionsUnitTest. They can be combined into one file.

Steps to reproduce

Proposed resolution

Remaining tasks

Review
Commit

User interface changes

API changes

Data model changes

Release notes snippet

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

quietone created an issue. See original summary.

quietone’s picture

Assigned: quietone » Unassigned
Issue summary: View changes
FileSize
5.05 KB

Something like this seems much easier to understand.

longwave’s picture

+++ b/core/modules/hal/tests/src/Unit/FieldNormalizerDenormalizeExceptionsUnitTest.php
@@ -27,4 +28,38 @@ public function testFieldNormalizerDenormalizeExceptions($context) {
+   * @coversDefaultClass \Drupal\hal\Normalizer\FieldItemNormalizer

@coversDefaultClass is only for class annotations, use @covers for method annotations.

quietone’s picture

@longwave, thanks.

I also didn't change the extends from the old base class to UnitTestCase. I then decided to change the file name as well to remove Unit' since this is in the Drupal\Tests\hal\Unit namespace. Because of the name change the interdiff was useless so I have omitted it and the diff wasn't any better. So just a new patch.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

BhumikaVarshney’s picture

Hi @quietone,
The above patch works for me.
3 files are deleted and 1 new file is created.
Thanks!!
RTBC+1

longwave’s picture

Title: Combine tests using NormalizerDenormalizeExceptionsUnitTestBas » Combine tests using NormalizerDenormalizeExceptionsUnitTestBase
Status: Needs review » Reviewed & tested by the community

This makes sense to me, it is much simpler to understand these tests now.

The last submitted patch, 2: 3210898-2.patch, failed testing. View results

quietone’s picture

I notice what is probably a random failure on the 9.3.x patch. I've decided to re-upload the patch so testing is against 9.3.x.

  • catch committed 00b7574 on 9.3.x
    Issue #3210898 by quietone, BhumikaVarshney: Combine tests using...

  • catch committed 22a5978 on 9.2.x
    Issue #3210898 by quietone, BhumikaVarshney: Combine tests using...
catch’s picture

Version: 9.3.x-dev » 9.2.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 9.3.x and cherry-picked to 9.2.x. Messed up the commit credit in the commit, but adding @longwave in the issue.

Status: Fixed » Closed (fixed)

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