Просмотр исходного кода

minor #3444 IntegrationTest - ensure tests in priority dir are priority tests indeed (keradus)

This PR was squashed before being merged into the 2.2 branch (closes #3444).

Discussion
----------

IntegrationTest - ensure tests in priority dir are priority tests indeed

In current state, this PR is expected to fail.

Raised by https://github.com/FriendsOfPHP/PHP-CS-Fixer/pull/3436#discussion_r161375420 .

We do have integration tests. What for ?
Sometimes we are not sure how 2+ fixers would play together, so we do make an integration test between them.

Integration test runner is smart enough to double check priorities:
 - if priorities are the same, the expected outcome must be the same regardless of order that fixers would be applied,
- if priorities are different, we don't care.

Now the same, but from fixed code point of view:
- if `$fixedInputCode === $fixedInputCodeWithReversedFixers`, then priorities doesn't matter,
- if `$fixedInputCode !== $fixedInputCodeWithReversedFixers`, then priorities have to be different.

This is all what matters for end-user.
This is all that's needed to be stable, and whenever 2+ fixers becomes priority-related or stop being one, the tests are checking they are still running together.
This is all that is needed for devs writing custom rules.

-------------------------

Now, at this repo we went one step forward.
We do know by heart that some of the fixers have priority conflicts and we created auto review for them that checks explicit priority and fact that priority itest exist, as addition to itest itself (ref `\PhpCsFixer\Tests\AutoReview\FixerFactoryTest`).
It was super important at beginning (and still important, yet not that much), when we basically didn't have any itest at all. This explicit priority comparison was the only point when we were checking the order of fixers is what we assumed, even when it was not tested by itest.

Now, theoretically, we could drop distinguishes between `misc` and `priority` itests, then we shall reject this PR, or we could keep it and finish this PR.
There is a lot of boilerplate added in this PR that allows itests to be extended even more, yet don't focus on the implementation yet, first let us discuss the idea behind this PR.
To ease what is a real change, I assumed we are using `tests\test` as external package that have to be generic, and adjust project-specific tests in our `IntegrationTest` directly. I also added 2 notes to put focus on that few lines that are project-specific.

Also, keep in mind that apparently not all of `priority` tests are passing new check, what is proving that they (the few that are not passing) are not priority tests in the end...

Commits
-------

b6a9c1e5 IntegrationTest - ensure tests in priority dir are priority tests indeed
Dariusz Ruminski 7 лет назад
Родитель
Сommit
5226b54ac6

+ 2 - 1
.gitattributes

@@ -1,4 +1,4 @@
-# @TODO 3.0 replace following `tests/... exportignore` with single `tests/ export-ignore`
+# @TODO 3.0 replace following `tests/... export-ignore` with single `tests/ export-ignore`
 tests/**/*Test.php export-ignore
 tests/AbstractDoctrineAnnotationFixerTestCase.php export-ignore
 tests/Differ/AbstractDifferTestCase.php export-ignore
@@ -6,6 +6,7 @@ tests/Fixtures/ export-ignore
 tests/Linter/AbstractLinterTestCase.php export-ignore
 tests/Report/AbstractReporterTestCase.php export-ignore
 tests/Test/AbstractTransformerTestCase.php export-ignore
+tests/Test/InternalIntegrationCaseFactory.php export-ignore
 
 .appveyor.yml export-ignore
 .composer-require-checker.json export-ignore

+ 11 - 2
.travis.yml

@@ -35,8 +35,17 @@ jobs:
                 - composer info -D | sort
             script:
                 # @TODO remove at 3.0
-                - git archive -o /dev/null HEAD -v 2>&1 | grep tests | grep \.php | grep -v tests/TestCase.php | grep -v tests/Test/Assert/AssertTokensTrait.php | grep -v tests/Test/AbstractFixerTestCase.php | grep -v tests/Test/AbstractIntegrationTestCase.php | grep -v tests/Test/IntegrationCase.php | grep -v tests/Test/IntegrationCaseFactory.php && (echo "UNKNOWN FILES DETECTED" && travis_terminate 1) || echo "NO UNKNOWN FILES"
-
+                - |
+                  git archive -o /dev/null HEAD -v 2>&1 | grep tests | grep \.php \
+                    | grep -v tests/Test/AbstractFixerTestCase.php \
+                    | grep -v tests/Test/AbstractIntegrationCaseFactory.php \
+                    | grep -v tests/Test/AbstractIntegrationTestCase.php \
+                    | grep -v tests/Test/Assert/AssertTokensTrait.php \
+                    | grep -v tests/Test/IntegrationCase.php \
+                    | grep -v tests/Test/IntegrationCaseFactory.php \
+                    | grep -v tests/Test/IntegrationCaseFactoryInterface.php \
+                    | grep -v tests/TestCase.php \
+                    && (echo "UNKNOWN FILES DETECTED" && travis_terminate 1) || echo "NO UNKNOWN FILES"
                 - ./check_trailing_spaces.sh || travis_terminate 1
                 - ./dev-tools/vendor/bin/composer-require-checker check composer.json --config-file=.composer-require-checker.json
 

+ 3 - 1
composer.json

@@ -58,9 +58,11 @@
         "classmap": [
             "tests/TestCase.php",
             "tests/Test/AbstractFixerTestCase.php",
+            "tests/Test/AbstractIntegrationCaseFactory.php",
             "tests/Test/AbstractIntegrationTestCase.php",
             "tests/Test/IntegrationCase.php",
-            "tests/Test/IntegrationCaseFactory.php"
+            "tests/Test/IntegrationCaseFactory.php",
+            "tests/Test/IntegrationCaseFactoryInterface.php"
         ]
     },
     "autoload-dev": {

+ 1 - 1
src/Fixer/Phpdoc/PhpdocNoUselessInheritdocFixer.php

@@ -45,7 +45,7 @@ final class PhpdocNoUselessInheritdocFixer extends AbstractFixer
      */
     public function getPriority()
     {
-        // Should run before NoEmptyPhpdocFixer, PhpdocInlineTagFixer and NoTrailingWhitespaceInCommentFixer
+        // Should run before NoEmptyPhpdocFixer, NoTrailingWhitespaceInCommentFixer
         // and after PhpdocToCommentFixer.
         return 6;
     }

+ 0 - 2
tests/AutoReview/FixerFactoryTest.php

@@ -143,7 +143,6 @@ final class FixerFactoryTest extends TestCase
             array($fixers['phpdoc_no_package'], $fixers['phpdoc_trim']),
             array($fixers['phpdoc_no_useless_inheritdoc'], $fixers['no_empty_phpdoc']),
             array($fixers['phpdoc_no_useless_inheritdoc'], $fixers['no_trailing_whitespace_in_comment']),
-            array($fixers['phpdoc_no_useless_inheritdoc'], $fixers['phpdoc_inline_tag']),
             array($fixers['phpdoc_order'], $fixers['phpdoc_separation']),
             array($fixers['phpdoc_order'], $fixers['phpdoc_trim']),
             array($fixers['phpdoc_separation'], $fixers['phpdoc_trim']),
@@ -162,7 +161,6 @@ final class FixerFactoryTest extends TestCase
             array($fixers['single_import_per_statement'], $fixers['no_multiline_whitespace_before_semicolons']),
             array($fixers['single_import_per_statement'], $fixers['no_singleline_whitespace_before_semicolons']),
             array($fixers['single_import_per_statement'], $fixers['no_unused_imports']),
-            array($fixers['single_import_per_statement'], $fixers['ordered_imports']),
             array($fixers['single_import_per_statement'], $fixers['space_after_semicolon']),
             array($fixers['unary_operator_spaces'], $fixers['not_operator_with_space']),
             array($fixers['unary_operator_spaces'], $fixers['not_operator_with_successor_space']),

+ 1 - 0
tests/AutoReview/ProjectCodeTest.php

@@ -216,6 +216,7 @@ final class ProjectCodeTest extends TestCase
         $rc = new \ReflectionClass($className);
 
         $this->assertTrue(
+            $rc->isInterface() || // due to hhvm only, @TODO remove me whem hhvm support is dropped
             $rc->isAbstract() || $rc->isFinal(),
             sprintf('Test class %s should be abstract or final.', $className)
         );

+ 0 - 0
tests/Fixtures/Integration/priority/phpdoc_no_useless_inheritdoc,phpdoc_inline_tag.test → tests/Fixtures/Integration/misc/phpdoc_no_useless_inheritdoc,phpdoc_inline_tag.test


+ 0 - 0
tests/Fixtures/Integration/priority/single_import_per_statement,ordered_imports.test → tests/Fixtures/Integration/misc/single_import_per_statement,ordered_imports.test


+ 31 - 0
tests/IntegrationTest.php

@@ -13,6 +13,8 @@
 namespace PhpCsFixer\Tests;
 
 use PhpCsFixer\Tests\Test\AbstractIntegrationTestCase;
+use PhpCsFixer\Tests\Test\IntegrationCase;
+use PhpCsFixer\Tests\Test\InternalIntegrationCaseFactory;
 
 /**
  * Test that parses and runs the fixture '*.test' files found in '/Fixtures/Integration'.
@@ -40,4 +42,33 @@ final class IntegrationTest extends AbstractIntegrationTestCase
     {
         return self::getFixturesDir().DIRECTORY_SEPARATOR.'.tmp.php';
     }
+
+    /**
+     * {@inheritdoc}
+     */
+    protected static function createIntegrationCaseFactory()
+    {
+        return new InternalIntegrationCaseFactory();
+    }
+
+    /**
+     * {@inheritdoc}
+     */
+    protected static function assertRevertedOrderFixing(IntegrationCase $case, $fixedInputCode, $fixedInputCodeWithReversedFixers)
+    {
+        parent::assertRevertedOrderFixing($case, $fixedInputCode, $fixedInputCodeWithReversedFixers);
+
+        $settings = $case->getSettings();
+
+        if (!isset($settings['isExplicitPriorityCheck'])) {
+            static::markTestIncomplete('Missing `isExplicitPriorityCheck` extension setting.');
+        }
+
+        if ($settings['isExplicitPriorityCheck']) {
+            static::assertTrue(
+                $fixedInputCode !== $fixedInputCodeWithReversedFixers,
+                sprintf('Test "%s" in "%s" is expected to be priority check.', $case->getTitle(), $case->getFileName())
+            );
+        }
+    }
 }

+ 271 - 0
tests/Test/AbstractIntegrationCaseFactory.php

@@ -0,0 +1,271 @@
+<?php
+
+/*
+ * This file is part of PHP CS Fixer.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *     Dariusz Rumiński <dariusz.ruminski@gmail.com>
+ *
+ * This source file is subject to the MIT license that is bundled
+ * with this source code in the file LICENSE.
+ */
+
+namespace PhpCsFixer\Tests\Test;
+
+use PhpCsFixer\RuleSet;
+use Symfony\Component\Finder\SplFileInfo;
+
+/**
+ * @author Dariusz Rumiński <dariusz.ruminski@gmail.com>
+ *
+ * @internal
+ */
+abstract class AbstractIntegrationCaseFactory implements IntegrationCaseFactoryInterface
+{
+    /**
+     * @param SplFileInfo $file
+     *
+     * @return IntegrationCase
+     */
+    public function create(SplFileInfo $file)
+    {
+        try {
+            if (!preg_match(
+                '/^
+                            --TEST--           \r?\n(?<title>          .*?)
+                       \s   --RULESET--        \r?\n(?<ruleset>        .*?)
+                    (?:\s   --CONFIG--         \r?\n(?<config>         .*?))?
+                    (?:\s   --SETTINGS--       \r?\n(?<settings>       .*?))?
+                    (?:\s   --REQUIREMENTS--   \r?\n(?<requirements>   .*?))?
+                    (?:\s   --EXPECT--         \r?\n(?<expect>         .*?\r?\n*))?
+                    (?:\s   --INPUT--          \r?\n(?<input>          .*))?
+                $/sx',
+                $file->getContents(),
+                $match
+            )) {
+                throw new \InvalidArgumentException('File format is invalid.');
+            }
+
+            $match = array_merge(
+                array(
+                    'config' => null,
+                    'settings' => null,
+                    'requirements' => null,
+                    'expect' => null,
+                    'input' => null,
+                ),
+                $match
+            );
+
+            return new IntegrationCase(
+                $file->getRelativePathname(),
+                $this->determineTitle($file, $match['title']),
+                $this->determineSettings($file, $match['settings']),
+                $this->determineRequirements($file, $match['requirements']),
+                $this->determineConfig($file, $match['config']),
+                $this->determineRuleset($file, $match['ruleset']),
+                $this->determineExpectedCode($file, $match['expect']),
+                $this->determineInputCode($file, $match['input'])
+            );
+        } catch (\InvalidArgumentException $e) {
+            throw new \InvalidArgumentException(
+                sprintf('%s Test file: "%s".', $e->getMessage(), $file->getRelativePathname()),
+                $e->getCode(),
+                $e
+            );
+        }
+    }
+
+    /**
+     * Parses the '--CONFIG--' block of a '.test' file.
+     *
+     * @param SplFileInfo $file
+     * @param string      $config
+     *
+     * @return array
+     */
+    protected function determineConfig(SplFileInfo $file, $config)
+    {
+        $parsed = $this->parseJson($config, array(
+            'indent' => '    ',
+            'lineEnding' => "\n",
+        ));
+
+        if (!is_string($parsed['indent'])) {
+            throw new \InvalidArgumentException(sprintf(
+                'Expected string value for "indent", got "%s".',
+                is_object($parsed['indent']) ? get_class($parsed['indent']) : gettype($parsed['indent']).'#'.$parsed['indent']
+            ));
+        }
+
+        if (!is_string($parsed['lineEnding'])) {
+            throw new \InvalidArgumentException(sprintf(
+                'Expected string value for "lineEnding", got "%s".',
+                is_object($parsed['lineEnding']) ? get_class($parsed['lineEnding']) : gettype($parsed['lineEnding']).'#'.$parsed['lineEnding']
+            ));
+        }
+
+        return $parsed;
+    }
+
+    /**
+     * Parses the '--REQUIREMENTS--' block of a '.test' file and determines requirements.
+     *
+     * @param SplFileInfo $file
+     * @param string      $config
+     *
+     * @return array
+     */
+    protected function determineRequirements(SplFileInfo $file, $config)
+    {
+        $parsed = $this->parseJson($config, array(
+            'hhvm' => true,
+            'php' => PHP_VERSION_ID,
+        ));
+
+        if (!is_int($parsed['php']) || $parsed['php'] < 50306) {
+            throw new \InvalidArgumentException(sprintf(
+                'Expected int >= 50306 value for "php", got "%s".',
+                is_object($parsed['php']) ? get_class($parsed['php']) : gettype($parsed['php']).'#'.$parsed['php']
+            ));
+        }
+
+        if (!is_bool($parsed['hhvm'])) {
+            throw new \InvalidArgumentException(sprintf(
+                'Expected bool value for "hhvm", got "%s".',
+                is_object($parsed['hhvm']) ? get_class($parsed['hhvm']) : gettype($parsed['hhvm']).'#'.$parsed['hhvm']
+            ));
+        }
+
+        return $parsed;
+    }
+
+    /**
+     * Parses the '--RULESET--' block of a '.test' file and determines what fixers should be used.
+     *
+     * @param SplFileInfo $file
+     * @param string      $config
+     *
+     * @return RuleSet
+     */
+    protected function determineRuleset(SplFileInfo $file, $config)
+    {
+        return new RuleSet($this->parseJson($config));
+    }
+
+    /**
+     * Parses the '--TEST--' block of a '.test' file and determines title.
+     *
+     * @param SplFileInfo $file
+     * @param string      $config
+     *
+     * @return string
+     */
+    protected function determineTitle(SplFileInfo $file, $config)
+    {
+        return $config;
+    }
+
+    /**
+     * Parses the '--SETTINGS--' block of a '.test' file and determines settings.
+     *
+     * @param SplFileInfo $file
+     * @param string      $config
+     *
+     * @return array
+     */
+    protected function determineSettings(SplFileInfo $file, $config)
+    {
+        $parsed = $this->parseJson($config, array(
+            'checkPriority' => true,
+        ));
+
+        if (!is_bool($parsed['checkPriority'])) {
+            throw new \InvalidArgumentException(sprintf(
+                'Expected bool value for "checkPriority", got "%s".',
+                is_object($parsed['checkPriority']) ? get_class($parsed['checkPriority']) : gettype($parsed['checkPriority']).'#'.$parsed['checkPriority']
+            ));
+        }
+
+        return $parsed;
+    }
+
+    /**
+     * @param SplFileInfo $file
+     * @param null|string $code
+     *
+     * @return string
+     */
+    protected function determineExpectedCode(SplFileInfo $file, $code)
+    {
+        $code = $this->determineCode($file, $code, '-out.php');
+
+        if (null === $code) {
+            throw new \InvalidArgumentException('Missing expected code.');
+        }
+
+        return $code;
+    }
+
+    /**
+     * @param SplFileInfo $file
+     * @param null|string $code
+     *
+     * @return null|string
+     */
+    protected function determineInputCode(SplFileInfo $file, $code)
+    {
+        return $this->determineCode($file, $code, '-in.php');
+    }
+
+    /**
+     * @param SplFileInfo $file
+     * @param null|string $code
+     * @param string      $suffix
+     *
+     * @return null|string
+     */
+    private function determineCode(SplFileInfo $file, $code, $suffix)
+    {
+        if (null !== $code) {
+            return $code;
+        }
+
+        $candidateFile = new SplFileInfo($file->getPathname().$suffix, '', '');
+        if ($candidateFile->isFile()) {
+            return $candidateFile->getContents();
+        }
+    }
+
+    /**
+     * @param null|string $encoded
+     * @param null|array  $template
+     *
+     * @return array
+     */
+    private function parseJson($encoded, array $template = null)
+    {
+        // content is optional if template is provided
+        if (!$encoded && null !== $template) {
+            $decoded = array();
+        } else {
+            $decoded = json_decode($encoded, true);
+
+            if (JSON_ERROR_NONE !== json_last_error()) {
+                throw new \InvalidArgumentException(sprintf('Malformed JSON: "%s", error: "%s".', $encoded, json_last_error_msg()));
+            }
+        }
+
+        if (null !== $template) {
+            $decoded = array_merge(
+                $template,
+                array_intersect_key(
+                    $decoded,
+                    array_flip(array_keys($template))
+                )
+            );
+        }
+
+        return $decoded;
+    }
+}

Некоторые файлы не были показаны из-за большого количества измененных файлов