Fix string array keys in data sets returned by data provider methods that do not match the parameter names in Kernel tests

pradhumanjain2311 → made their first commit to this issue’s fork.

Fixes the Kernel tests + one Unit test that was missed in 📌 Fix string array keys in data sets returned by data provider methods that do not match the parameter names in Unit tests Fixed .

Reviewing the changes and they appear to make sense. Assuming these would be caught automatically once we upgrade?

Yes, they result in deprecation errors in PHPUnit 10. See test failures in the upgrade issue.

Updating PHPUnit compatibility due to the deprecation in version 10.5.18, which now requires string array keys in data sets returned by data provider methods to match parameter names in Kernel tests. This change ensures continued compatibility with the latest PHPUnit version and maintains best practices for data provider usage

The scope of this MR is kind of big (500ish LOC to review), especially given that one has to open up the file to check that that the parameter names actually match the keys. That said, I don't have a better suggestion, so going ahead with reviewing it now.

@mondrake I assume you programmatically scanned for these somehow? Could you document it on the issue for posterity?

If I were to rescope this issue, I would put all the migrate stuff in one issue (since the expected parameters aren't even in the same file), and the rest of the changes in the first issue.

#11 PHPUnit 10.5.18 triggers a deprecation for any key not matching the test method argument name. We do not see it in this MR but that is visible in the latest test run of 📌 Upgrade PHPUnit to 10, drop Symfony PHPUnit-bridge dependency Fixed which is the input to this MR.

So far, I reviewed all the non-Migrate changes and confirmed that each change matches the expected parameters. I did not check for missed invalid keys.

Paused in my review based on feedback from @larowlan.

I think we should press ahead on this, so we can get to phpunit 10, but we should open a followup to remove these keys.

I don't think they provide value like the cases names do.

Tagging for #16.

#16 is the opposite of #3443535-9: Change @dataprovider to static in SqlContentEntityStorageSchemaTest → , so before any follow up there needs to be a decision made IMHO. A coding standard issue this one?

Personally I think they can help in some cases, especially when data providers pass booleans, like @longwave said in the other issue.

@mondrake Which job is it on the linked issue? (Sorry, GitLab CI is a scavenger hunt sometimes.)

FWIW I also find keys helpful, so agreed that we need to have a coding-standards-level discussion about it. I just defer to @larowlan in this since he's a framework manager and also involved in coding standards. :)

For #19, a code snippet showing the fails would also be helpful here.

#19 see e.g. https://git.drupalcode.org/issue/drupal-3417066/-/jobs/1467574

I think we need to have the discussion, which is another reason not to hold this up. We can always come back afterwards. I'll add to the agenda for next coding standards meeting.

Added to #3443047: Coding Standards Meeting Wednesday 2024-05-08 0900 UTC →

Thanks @mondrake. So e.g.:

    1) Drupal\KernelTests\Core\Entity\ContentEntityStorageBaseTest::testCreate
    with data set #0 ('test_bundle')
    * Providing invalid named argument $scalar for method
    Drupal\KernelTests\Core\Entity\ContentEntityStorageBaseTest::testCreate()
    is deprecated and will not be supported in PHPUnit 11.0.
    
    * Providing invalid named argument $scalar for method
    Drupal\KernelTests\Core\Entity\ContentEntityStorageBaseTest::testCreate()
    is deprecated and will not be supported in PHPUnit 11.0.

Docblock and signature from the MigrateSqlSourceTestBase base class for several of the updated tests:

 /**
   * Tests the source plugin against a particular data set.
   *
   * @param array $source_data
   *   The source data that the plugin will read. See getDatabase() for the
   *   expected format.
   * @param array $expected_data
   *   The result rows the plugin is expected to return.
   * @param int $expected_count
   *   (optional) How many rows the source plugin is expected to return.
   * @param array $configuration
   *   (optional) Configuration for the source plugin.
   * @param mixed $high_water
   *   (optional) The value of the high water field.
   * @param string|null $expected_cache_key
   *   (optional) The expected cache key.
   *
   * @dataProvider providerSource
   *
   * @requires extension pdo_sqlite
   */
  public function testSource(array $source_data, array $expected_data, $expected_count = NULL, array $configuration = [], $high_water = NULL, $expected_cache_key = NULL) {

core/modules/migrate_drupal/tests/src/Kernel/NodeMigrationTypePluginAlterTest.php is weird and breaks the pattern, but that is because testMigrationPluginAlter($type, array $migration_definitions, array $expected) names its parameters in a way that's inconsistent with the above base class.

I finished reviewing all the files to make sure the new keys match what's expected for the test method parameter.

I didn't search to validate that all instances have been changed. If any were missed, or additional ones crop up, they'll show up when we adopt PHPUnit 10 and can be cleaned up at that time.

Untagged for followup since it's been added to the coding standards agenda.

@pradhumanjain2311, to get credit for resolving a merge conflict, you need to document the merge conflict on the issue. Reference: https://www.drupal.org/about/core/policies/maintainers/how-is-credit-gra... →

Resolving a merge conflict when rerolling, rebasing, or merging HEAD
Document the merge conflict diff on the issue in a <code> tag.

Saving issue credits.

Committed to 11.x. Thanks! I also cherry-picked it to 10.3.x as a no-op change to tests.

I considered whether this needed a CR, but since PHPUnit will enforce this for us in the future, there's no needs.

Thanks everyone!

xjm → closed merge request !7611

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