Incorrect method names for dataProviders

I don't think that hurts anyone.

assigning to myself

Hi,
I am new to contributing to open source, but have been using the software for a while, so its time to give back,
This should have renamed all of the @dataprovider 's methods and docblocks to the standard described here,
Thanks,
Jacob

Sorry accidently missed the d in provider in the method providerLocalePageRoutes()
EDIT: Wrong patch name

Correct patch name for post number

Marking 'needs review' to get the test bot to fire

looks like a simple typo again - proivderTaxonomyPageRoutes - note the spelling.

typo fixed

Have applied the patch and re-ran the script

grep -nriI "\* @dataProvider" * --exclude-dir=core/vendor | grep -v " provider"

No files found that do not conform to the documentation. The method names are mostly a rename of 'get' or 'test' to 'provider' which makes perfect sense.

All phpunit tests pass and the test bot is green. Happy to RTBC this issue.

#2057905: [policy, no patch] Discuss the standards for phpunit based tests is still a proposal, not policy.

FWIW, single-pass, fast PCRE:

$ grep -r --exclude-dir=vendor --include='*.php' -P '@dataProvider (?!provider)' core/

Renaming all of those to replace get|test|resolve|route|calculate|form|accept with a "provider" prefix is perfectly sensible, regardless of whether a more granular guideline may be discussed (or not, hopefully). Some affected method names will require manual adjustments.

Status quo, as of now:

core/modules/aggregator/tests/src/Menu/AggregatorLocalTasksTest.php:   * @dataProvider getAggregatorAdminRoutes
core/modules/aggregator/tests/src/Menu/AggregatorLocalTasksTest.php:   * @dataProvider getAggregatorSourceRoutes
core/modules/block_content/tests/src/Menu/BlockContentLocalTasksTest.php:   * @dataProvider getBlockContentListingRoutes
core/modules/book/tests/src/Menu/BookLocalTasksTest.php:   * @dataProvider getBookAdminRoutes
core/modules/book/tests/src/Menu/BookLocalTasksTest.php:   * @dataProvider getBookNodeRoutes
core/modules/config/tests/src/Menu/ConfigLocalTasksTest.php:   * @dataProvider getConfigAdminRoutes
core/modules/language/tests/src/Menu/LanguageLocalTasksTest.php:   * @dataProvider getLanguageAdminOverviewRoutes
core/modules/locale/tests/src/LocaleLookupTest.php:   * @dataProvider resolveCacheMissWithFallbackProvider
core/modules/locale/tests/src/Menu/LocaleLocalTasksTest.php:   * @dataProvider getLocalePageRoutes
core/modules/shortcut/tests/src/Menu/ShortcutLocalTasksTest.php:   * @dataProvider getShortcutPageRoutes
core/modules/simpletest/tests/src/TestBaseTest.php:   * @dataProvider randomStringValidateProvider
core/modules/system/tests/src/Menu/SystemLocalTasksTest.php:   * @dataProvider getSystemAdminRoutes
core/modules/taxonomy/tests/src/Menu/TaxonomyLocalTasksTest.php:   * @dataProvider getTaxonomyPageRoutes
core/modules/tour/tests/src/Entity/TourTest.php:   * @dataProvider routeProvider
core/modules/user/tests/src/Menu/UserLocalTasksTest.php:   * @dataProvider getUserAdminRoutes
core/modules/user/tests/src/Menu/UserLocalTasksTest.php:   * @dataProvider getUserLoginRoutes
core/modules/user/tests/src/Menu/UserLocalTasksTest.php:   * @dataProvider getUserPageRoutes
core/modules/views/tests/src/Entity/ViewTest.php:   * @dataProvider calculateDependenciesProvider
core/tests/Drupal/Tests/Core/EventSubscriber/ModuleRouteSubscriberTest.php:   * @dataProvider testRemoveRouteProvider
core/tests/Drupal/Tests/Core/Extension/ModuleHandlerTest.php:   * @dataProvider dependencyProvider
core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php:   * @dataProvider formStateResponseProvider
core/tests/Drupal/Tests/Core/Menu/LocalActionManagerTest.php:   * @dataProvider getActionsForRouteProvider
core/tests/Drupal/Tests/Core/Path/PathMatcherTest.php:   * @dataProvider getMatchPathData
core/tests/Drupal/Tests/Core/Route/RoleAccessCheckTest.php:   * @dataProvider roleAccessProvider
core/tests/Drupal/Tests/Core/Routing/AcceptHeaderMatcherTest.php:   * @dataProvider acceptFilterProvider
core/tests/Drupal/Tests/Core/Routing/RouteMatchBaseTest.php:   * @dataProvider routeMatchProvider
core/tests/Drupal/Tests/Core/Routing/RouteMatchBaseTest.php:   * @dataProvider routeMatchProvider
core/tests/Drupal/Tests/Core/Routing/RouteMatchBaseTest.php:   * @dataProvider routeMatchProvider
core/tests/Drupal/Tests/Core/Routing/RouteMatchBaseTest.php:   * @dataProvider routeMatchProvider
core/tests/Drupal/Tests/Core/Routing/RouteMatchBaseTest.php:   * @dataProvider routeMatchProvider
core/tests/Drupal/Tests/Core/Routing/RouteMatchBaseTest.php:   * @dataProvider routeMatchProvider

OK I've tried apply @sun hints..

Go test bot, go!

Fixing one naming mistake, so applying updated patch. To be clear I follow @sun mini shell-grep test:
$ grep -r --exclude-dir=vendor --include='*.php' -P '@dataProvider (?!provider)' core/

Good work sobo3sh!

Some remarks about your patch:

1. In #2057905: [policy, no patch] Discuss the standards for phpunit based tests is some guidance for the naming of dataprovider functions. If the test function is named testSomeSpecialName() then the provider function is called providerSomeSpecialName(). That is not always done in your patch. There is an exception when a dataprovider is used multiple times.
2. #2303391: Tab 'Log in' shows on 'Request new password page for logged-in users and is missing for logged-out users has landed and now you need to reroll this patch. Sorry :-(

Rerolled.

@ankitgarg: Can you add an interdiff.txt file. That makes reviewing a lot easier. :-)

@daffie: Interdiff.txt Attached for comment #24

Hi ankitgarg!

1. You forgot a few cases in core/tests/Drupal/Tests/Core/Config/ConfigTest.php
2. Can you apply #22.1 to your patch

@ankitgarg any problems?

Rerolled on #24

Hi guys,

I rerolled the patch as it didn't apply properly anymore and took the chance to address some missing cases in ConfigTest.php as stated in #27.

I also had a look in cases like #22.1 but all the ones I found are exceptions where the dataprovider is used in multiple places.

Also the comments for testRandomStringValidate in TestBaseTest.php seems to be missing since commit 88350da6008f558dcd8f6855a003598e4544c4b8

It's added in this patch, so let me know if it should be removed.

Patch does not apply. :(

Great job! Here's 5 more though that need done!

core/modules/migrate_drupal/tests/src/Unit/MigrationStorageTest.php:24:   * @dataProvider entityIdsDataProvider
core/modules/simpletest/tests/src/Unit/TestInfoParsingTest.php:15:   * @dataProvider infoParserProvider
core/tests/Drupal/Tests/Component/Bridge/ZfExtensionManagerSfContainerTest.php:68:   * @dataProvider canonicalizeNameProvider
core/tests/Drupal/Tests/Core/Cache/CacheTest.php:112:   * @dataProvider mergeMaxAgesProvider
core/tests/Drupal/Tests/Core/ContentNegotiationTest.php:49:   * @dataProvider priorityFormatProvider

Hi guys,

Here is another patch addressing #34

Attaching interdiff as well.

Cheers,

Rerolled the patch.

It still needs a beta evaluation.

The patch looks good, but it does need a re-roll.

I searched for occurrences of @dataProvider (?!provider) (a regular expression) in the code base and there are two more incorrect data provider method names left, namely \Drupal\Tests\Core\Cache\CacheContextsTest::validateTokensProvider() and \Drupal\Tests\Core\Validation\Plugin\Validation\Constraint\PrimitiveTypeConstraintValidatorTest::provideTestValidate().

Rerolled #42 as per the latest release.

Removing Needs reroll tag from this task.

Looks good. Some minor comments:

+++ b/core/modules/simpletest/tests/src/Unit/TestBaseTest.php
@@ -79,6 +79,15 @@ public function providerRandomStringValidate() {
+   * Tests the random strings validation rules.
+   *
+   * @param string $string
+   *   The string to validate.
+   * @param bool $expected
+   *   The expected result of the validation.
+   *
+   * @see \Drupal\simpletest\TestBase::randomStringValidate().
+   *

This can go. The first and the last line are not necessary because of @covers ::randomStringValidate. The parameter definition is not necessary because they are documented with the provider function.

+++ b/core/modules/system/tests/modules/accept_header_routing_test/tests/Unit/AcceptHeaderMatcherTest.php
@@ -71,7 +71,7 @@ public function acceptFilterProvider() {
-   * @dataProvider acceptFilterProvider
+   * @dataProvider providerAcceptFilter

The provider function must be renamed too.

+++ b/core/tests/Drupal/Tests/Core/ContentNegotiationTest.php
@@ -44,7 +44,30 @@ public function testAjaxIframeUpload() {
-   * Tests the specifying a format via query parameters gets used.
+   * Tests the getContentType() method when a priority format is found.
+   *
+   * @dataProvider providerPriorityFormatIsFound
+   * @covers ::getContentType
+   */
+  public function testAPriorityFormatIsFound($priority, $format) {
+    $request = new Request();
+    $request->setFormat($format['format'], $format['mime_type']);
+    $request->headers->set('Accept', sprintf('%s,application/json', $format['mime_type']));
+
+    $this->assertSame($priority, $this->contentNegotiation->getContentType($request));
+  }
+
+  public function providerPriorityFormatIsFound()
+  {
+    return [
+      ['html', ['format' => 'html', 'mime_type' => 'text/html']],
+    ];
+  }
+
+  /**
+   * Tests the getContentType() method when no priority format is found but a valid one is found.
+   *
+   * @covers ::getContentType

Adding a new test is fine. It just does not belong in this issue.

Rerolled.
2 files relevant to patch removed:
core/modules/migrate_drupal/tests/src/Unit/MigrationStorageTest.php (from issue #2549013)
core/tests/Drupal/Tests/Core/ContentNegotiationTest.php (from issue #2506533)

Renamed acceptFilterProvider to providerAcceptFilter.
Left docs for testRandomStringValidate().

are we still missing these?

modules/migrate/tests/src/Unit/MigrateSqlIdMapTest.php:371:   * @dataProvider lookupDestinationIdMappingDataProvider
modules/migrate/tests/src/Unit/MigrateSqlIdMapTest.php:455:   * @dataProvider lookupSourceIDMappingDataProvider
modules/migrate/tests/src/Unit/MigrateSqlIdMapTest.php:573:   * @dataProvider updateCountDataProvider
modules/migrate/tests/src/Unit/MigrateSqlIdMapTest.php:615:   * @dataProvider errorCountDataProvider
modules/node/src/Tests/Migrate/d7/MigrateNodeTypeTest.php:42:   * @dataProvider testNodeTypeDataProvider
tests/Drupal/Tests/Component/DependencyInjection/ContainerTest.php:666:   * @dataProvider scopeExceptionTestProvider
tests/Drupal/Tests/Component/DependencyInjection/Dumper/OptimizedPhpArrayDumperTest.php:99:     * @dataProvider getAliasesDataProvider
tests/Drupal/Tests/Component/DependencyInjection/Dumper/OptimizedPhpArrayDumperTest.php:138:     * @dataProvider getParametersDataProvider
tests/Drupal/Tests/Component/DependencyInjection/Dumper/OptimizedPhpArrayDumperTest.php:210:     * @dataProvider getDefinitionsDataProvider
tests/Drupal/Tests/Core/Cache/Context/CacheContextsManagerTest.php:209:   * @dataProvider validateTokensProvider
tests/Drupal/Tests/Core/Form/FormElementHelperTest.php:28:   * @dataProvider getElementByNameProvider
tests/Drupal/Tests/Core/Form/FormElementHelperTest.php:113:   * @dataProvider getElementTitleProvider

Renamed remaining files.
Removed testNodeTypeDataProvider from MigrateNodeTypeTest.php since it's not declared anywhere.

Looks like we have them all now, let's see if this comes back green...

All changes look good to me.

+1

@tomasnagy I assume you're not still working on it.

Reroll and update.

Cleaning up drupalcampfi tags.

Ok, this looks good now, this is RTBC imo.

Rerolled patch #66 . Hope it works this time!

I manually reviewed the latest patch and looking the scope of the issue all seems to be OK.
RTBC I'd say.

So the problem is that #2057905: [policy, no patch] Discuss the standards for phpunit based tests has not been ratified and accepted yet. So making this change is premature. One thing that would be good is to get phpcs rules developed for the proposed standard so that once we've agreed the standard then we can implemented the rules in core and not regress. I realise that this will make the work here seem currently wasted. But the problem with all coding standards outside of the work to enforce them using phpcs.xml.dist is impossible to enforce.

Postponing this issue on #2057905: [policy, no patch] Discuss the standards for phpunit based tests

Adding coding standards tag and changing component to 'other'.