Skip to content

Commit

Permalink
Make static fixes configurable (WIP)
Browse files Browse the repository at this point in the history
  • Loading branch information
andrewnicols committed Sep 16, 2023
1 parent b86c003 commit c89163a
Show file tree
Hide file tree
Showing 8 changed files with 147 additions and 55 deletions.
8 changes: 5 additions & 3 deletions moodle-extra/ruleset.xml
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,6 @@
-->

<rule ref="moodle.Classes.UnitTestFormatting"/>

<!--
Detect issues with Unit Test dataProviders:
- private providers
Expand All @@ -95,6 +93,10 @@
- dataProviders which do not return an array or Iterable
- dataProviders which can be converted to a static method (PHPUnit 10 compatibility)
-->
<rule ref="moodle.PHPUnit.TestCaseProvider"/>
<rule ref="moodle.PHPUnit.TestCaseProvider">
<properties>
<property name="autofixStaticProviders" value="true"/>
</properties>
</rule>

</ruleset>
35 changes: 23 additions & 12 deletions moodle/Sniffs/PHPUnit/TestCaseProviderSniff.php
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,13 @@
*/
class TestCaseProviderSniff implements Sniff {

/**
* Whether to autofix static providers.
*
* @var bool
*/
public $autofixStaticProviders = false;

/**
* Register for open tag (only process once per file).
*/
Expand Down Expand Up @@ -274,23 +281,27 @@ protected function checkDataProvider(

// In preparation for PHPUnit 10, we want to recommend that data providers are statically defined.
if (!$methodProps['is_static']) {
// We can make this fixable if the method does not contain any `$this`.
// Search the body.
$hasThis = false;
$bodyStart = $tokens[$providerPointer]['scope_opener'] + 1;
$bodyEnd = $tokens[$providerPointer]['scope_closer'] - 1;
while ($token = $file->findNext(T_VARIABLE, $bodyStart, $bodyEnd)) {
if ($tokens[$token]['content'] === '$this') {
$hasThis = true;
break;
$supportAutomatedFix = true;
if (!$this->autofixStaticProviders) {
$supportAutomatedFix = false;
} else {
// We can make this fixable if the method does not contain any `$this`.
// Search the body.
$bodyStart = $tokens[$providerPointer]['scope_opener'] + 1;
$bodyEnd = $tokens[$providerPointer]['scope_closer'] - 1;
while ($token = $file->findNext(T_VARIABLE, $bodyStart, $bodyEnd)) {
if ($tokens[$token]['content'] === '$this') {
$supportAutomatedFix = false;
break;
}
}
}

if ($hasThis) {
if (!$supportAutomatedFix) {
$file->addWarning(
'Data provider method "%s" will need to be converted to static in future.',
$pointer + 2,
'dataProviderSyntaxMethodNotStatic',
'dataProviderNotStatic',
[
$methodName,
]
Expand All @@ -299,7 +310,7 @@ protected function checkDataProvider(
$fix = $file->addFixableWarning(
'Data provider method "%s" will need to be converted to static in future.',
$pointer + 2,
'dataProviderSyntaxMethodNotStatic',
'dataProviderNotStatic',
[
$methodName,
]
Expand Down
37 changes: 3 additions & 34 deletions moodle/Tests/MoodleCSBaseTestCase.php
Original file line number Diff line number Diff line change
Expand Up @@ -83,11 +83,11 @@ public function set_component_mapping(array $mapping): void {
protected function set_standard($standard) {
$installedStandards = \PHP_CodeSniffer\Util\Standards::getInstalledStandardDetails();

foreach (array_keys($installedStandards) as $standard) {
if (\PHP_CodeSniffer\Util\Standards::isInstalledStandard($standard) === false) {
foreach (array_keys($installedStandards) as $installedStandard) {
if (\PHP_CodeSniffer\Util\Standards::isInstalledStandard($installedStandard) === false) {
// They didn't select a valid coding standard, so help them
// out by letting them know which standards are installed.
$error = 'ERROR: the "'.$standard.'" coding standard is not installed. ';
$error = 'ERROR: the "'. $installedStandard.'" coding standard is not installed. ';
ob_start();
\PHP_CodeSniffer\Util\Standards::printInstalledStandards();
$error .= ob_get_contents();
Expand All @@ -96,37 +96,6 @@ protected function set_standard($standard) {
}
}
$this->standard = $standard;
return;
// Since 2.9 arbitrary standard directories are not allowed by default,
// only those under the CodeSniffer/Standards dir are detected. Other base
// dirs containing standards can be added using CodeSniffer.conf or the
// PHP_CODESNIFFER_CONFIG_DATA global (installed_paths setting).
// We are using the global way here to avoid changes in the phpcs import.
// phpcs:disable
if (!isset($GLOBALS['PHP_CODESNIFFER_CONFIG_DATA']['installed_paths'])) {
$localcodecheckerpath = realpath(__DIR__ . '/../');
$GLOBALS['PHP_CODESNIFFER_CONFIG_DATA'] = ['installed_paths' => $localcodecheckerpath];
}
// phpcs:enable

// Basic search of standards in the allowed directories.
$stdsearch = array(
__DIR__ . '/../phpcs/src/Standards', // PHPCS standards dir.
__DIR__ . '/..', // Plugin local_codechecker dir, allowed above via global.
);

foreach ($stdsearch as $stdpath) {
$stdpath = realpath($stdpath . '/' . $standard);
$stdfile = $stdpath . '/ruleset.xml';
if (file_exists($stdfile)) {
$this->standard = $stdpath; // Need to pass the path here.
break;
}
}
// Standard not found, fail.
if ($this->standard === null) {
$this->fail('Standard "' . $standard . '" not found.');
}
}

/**
Expand Down
18 changes: 14 additions & 4 deletions moodle/Tests/PHPUnitTestCaseProviderTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -38,28 +38,28 @@ class PHPUnitTestCaseProviderTest extends MoodleCSBaseTestCase {
public function provider_phpunit_data_providers() {
return [
'Correct' => [
'fixture' => 'fixtures/phpunit/provider/correct_test.php',
'fixture' => 'fixtures/PHPUnit/provider/correct_test.php',
'errors' => [],
'warnings' => [],
],
'Provider Visibility' => [
'fixture' => 'fixtures/phpunit/provider/provider_visibility_test.php',
'fixture' => 'fixtures/PHPUnit/provider/provider_visibility_test.php',
'errors' => [
12 => 'Data provider method "provider" must be public.',
],
'warnings' => [
],
],
'Provider Naming conflicts with test names' => [
'fixture' => 'fixtures/phpunit/provider/provider_prefix_test.php',
'fixture' => 'fixtures/PHPUnit/provider/provider_prefix_test.php',
'errors' => [
6 => 'Data provider must not start with "test_". "test_provider" provided.',
],
'warnings' => [
],
],
'Static Providers' => [
'fixture' => 'fixtures/phpunit/provider/static_providers_test.php',
'fixture' => 'fixtures/PHPUnit/provider/static_providers_test.php',
'errors' => [
],
'warnings' => [
Expand All @@ -68,6 +68,16 @@ public function provider_phpunit_data_providers() {
34 => 'Data provider method "partially_fixable_provider" will need to be converted to static in future.',
],
],
'Static Providers Applying fixes' => [
'fixture' => 'fixtures/PHPUnit/provider/static_providers_fix_test.php',
'errors' => [
],
'warnings' => [
13 => 'Data provider method "fixable_provider" will need to be converted to static in future.',
24 => 'Data provider method "unfixable_provider" will need to be converted to static in future.',
35 => 'Data provider method "partially_fixable_provider" will need to be converted to static in future.',
],
],
];
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
// phpcs:set moodle.PHPUnit.TestCaseProvider autofixStaticProviders true
<?php
defined('MOODLE_INTERNAL') || die(); // Make this always the 1st line in all CS fixtures.

// A class with 3 methods, using all the covers options correctly.

/**
* @coversDefaultClass \some\namespace\one
* @covers ::all()
*/
class static_providers_test extends base_test {
/**
* @dataProvider fixable_provider
*/
public function test_fixable(): void {
// Nothing to test.
}

public function fixable_provider(): array {
return [];
}

/**
* @dataProvider unfixable_provider
*/
public function test_unfixable_provider(): void {
// Nothing to test.
}

public function unfixable_provider(): array {
return $this->provider();
}

/**
* @dataProvider partially_fixable_provider
*/
public function test_partially_fixable(): void {
// Nothing to test.
}

public function partially_fixable_provider(): array {
$this->call_something();
return $this->fixable_provider();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
// phpcs:set moodle.PHPUnit.TestCaseProvider autofixStaticProviders true
<?php
defined('MOODLE_INTERNAL') || die(); // Make this always the 1st line in all CS fixtures.

// A class with 3 methods, using all the covers options correctly.

/**
* @coversDefaultClass \some\namespace\one
* @covers ::all()
*/
class static_providers_test extends base_test {
/**
* @dataProvider fixable_provider
*/
public function test_fixable(): void {
// Nothing to test.
}

public static function fixable_provider(): array {
return [];
}

/**
* @dataProvider unfixable_provider
*/
public function test_unfixable_provider(): void {
// Nothing to test.
}

public function unfixable_provider(): array {
return $this->provider();
}

/**
* @dataProvider partially_fixable_provider
*/
public function test_partially_fixable(): void {
// Nothing to test.
}

public function partially_fixable_provider(): array {
$this->call_something();
return self::fixable_provider();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ class static_providers_test extends base_test {
// Nothing to test.
}

public static function fixable_provider(): array {
public function fixable_provider(): array {
return [];
}

Expand All @@ -39,6 +39,6 @@ class static_providers_test extends base_test {

public function partially_fixable_provider(): array {
$this->call_something();
return self::fixable_provider();
return $this->fixable_provider();
}
}
10 changes: 10 additions & 0 deletions moodle/ruleset.xml
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,16 @@

<rule ref="Zend.Files.ClosingTag"/>

<!--
Detect issues with Unit Test dataProviders:
- private providers
- providers which do not exist
- providers whose name is prefixed with _test
- incorrect casing of dataProvider
- dataProviders which do not return an array or Iterable
-->
<rule ref="moodle.PHPUnit.TestCaseProvider"/>

<!-- Disable this exact error unless it's approved -->
<rule ref="moodle.Commenting.InlineComment.SpacingAfter">
<severity>0</severity>
Expand Down

0 comments on commit c89163a

Please sign in to comment.