Improve test coverage of \Drupal\Tests\layout_builder\Unit\SectionTest::testUnsetThirdPartySetting()

@Chris Burge, thanks for filing this.

I guess the original intention is to use the data provider, but somehow forgot or did not notice that it does not work at all.

So Instead of removing the data provider, let's add the arguments to testUnsetThirdPartySetting() and rewrite testUnsetThirdPartySetting() to make it work.

Tagging "Bug Smash Initiative"

Thanks for the patch.

1) I looked for similar test code and see the following which follows a similar pattern.

  /**
   * @covers ::getThirdPartySettings
   * @dataProvider providerTestGetThirdPartySettings
   */
  public function testGetThirdPartySettings($provider, $expected) {
    $this->assertSame($expected, $this->section->getThirdPartySettings($provider));
  }

  /**
   * Provides test data for ::testGetThirdPartySettings().
   */
  public function providerTestGetThirdPartySettings() {
    $data = [];
    $data[] = [
      'bad_judgement',
      ['blink_speed' => 'fast', 'spin_direction' => 'clockwise'],
    ];
...
  }

2) unsetThirdPartySetting takes arguments $provider and $key so that looks correct: public function unsetThirdPartySetting($provider, $key) {.

3) The goal is to make sure that the following data is provided in providerTestUnsetThirdPartySetting which is appears to do.

-    $this->section->unsetThirdPartySetting('bad_judgement', 'blink_speed');
-    $this->assertSame(['spin_direction' => 'clockwise'], $this->section->getThirdPartySettings('bad_judgement'));
-    $this->section->unsetThirdPartySetting('hunt_and_peck', 'delay');
-    $this->assertSame([], $this->section->getThirdPartySettings('hunt_and_peck'));
-    $this->section->unsetThirdPartySetting('bad_judgement', 'non_existing_key');
-    $this->section->unsetThirdPartySetting('non_existing_provider', 'non_existing_key');

The mapping is as such:

    $this->section->unsetThirdPartySetting('bad_judgement', 'blink_speed');
    $this->assertSame(['spin_direction' => 'clockwise'], $this->section->getThirdPartySettings('bad_judgement'));

is covered by:

    $data[] = [
      'bad_judgement',
      'blink_speed',
      [
        'spin_direction' => 'clockwise',
      ],
    ];

and

    $this->section->unsetThirdPartySetting('hunt_and_peck', 'delay');
    $this->assertSame([], $this->section->getThirdPartySettings('hunt_and_peck'));

is covered by:

    $data[] = [
      'hunt_and_peck',
      'delay',
      [],
    ];

and

$this->section->unsetThirdPartySetting('bad_judgement', 'non_existing_key');

is covered by:

    $data[] = [
      'bad_judgement',
      'non_existing_key',
      [],
    ];

and

$this->section->unsetThirdPartySetting('non_existing_provider', 'non_existing_key');

is covered by:

    $data[] = [
      'non_existing_provider',
      'non_existing_key',
      [],
    ];

4) If tests pass, then this appears RTBC to me.

Ah, this should be on 9.1.x. Kicking off tests for 9.1.x.

+++ b/core/modules/layout_builder/tests/src/Unit/SectionTest.php
@@ -323,36 +322,6 @@ class SectionTest extends UnitTestCase {
     $this->section->unsetThirdPartySetting('bad_judgement', 'non_existing_key');
     $this->section->unsetThirdPartySetting('non_existing_provider', 'non_existing_key');
}

I disagree with #2 because the last two lines of the test method make changes but then don't assert anything. I think the data provider adds some value here, reducing repetition and making this more like the other test methods in the class. Therefore I think #3 is the better option.

So Instead of removing the data provider, let's add the arguments to testUnsetThirdPartySetting() and rewrite testUnsetThirdPartySetting() to make it work.

I agree with this (#3).

And while we never NEED a dataProvider, it is a great way to document what is being tested. By separating the test data from the test methods it makes it easy to see problems with what we're testing, it helps to ensure consistency between test cases, and it makes it simple to extend the test should edge-case failures arise in the future. Plus with a provider failure of one data set won't prevent the other data sets from running ...

Also,

the last two lines of the test method make changes but then don't assert anything

That't true, and when put into a dataProvider these cases are now checked with assertions, and one of the cases was found to be incomplete so the data set had to be changed so the assertion would work. That proves the usefulness of the dataProvider - it detected something that we thought was being tested by really wasn't being tested. Now it is...

Besides, all the code is basically there already, this is just a minor modification of what's there, it just makes it better.

The only improvement I would suggest is to add dataset descriptions via array keys, that way instead of seeing something like "data set 2 failed" PHPUnit will show us the key of the dataset - choosing a descriptive string as the key also documents what each dataset is intended to test.

This was added in #2942661: Sections should have third-party settings, whoops.

Let's proceed with #3.

I agree with @TR about adding keys to the $data[] declarations (idk why we didn't do that in the first place).

With the keys, and an interdiff.

Even better now.

Correcting typo for testUnsetThirdPartySetting()

   /**
-   * Provides test data for ::testUnsetThirdPartySettings().
+   * Provides test data for ::testUnsetThirdPartySetting().
    */

Committed 84bae9d and pushed to 9.1.x. Thanks!

c/p to 9.0.x and 8.9.x as this is a bug fix and non-disruptive. Having the branches in sync will mean other backports are likely to apply cleanly.