From c7ed0ffa1a5f335f6521eb63598df520342132db Mon Sep 17 00:00:00 2001 From: Ben Thomson Date: Fri, 14 Aug 2020 08:00:39 +0800 Subject: [PATCH 1/4] Implement improved testing procedure (#5238) Refs: https://github.com/octobercms/library/commit/5feb7b872f2ab4e1e09b7980b61b2d7048d3fc17 --- .github/workflows/code-quality-pr.yaml | 14 ++-- .github/workflows/code-quality-push.yaml | 12 +-- .github/workflows/matchers/phpcs-matcher.json | 23 ------ .github/workflows/tests.yml | 74 +++++++++++++----- .github/workflows/utilities/phpcs-pr | 77 +++++++++++++++++++ .github/workflows/utilities/phpcs-push | 77 +++++++++++++++++++ modules/backend/helpers/Backend.php | 4 +- modules/cms/classes/CodeParser.php | 6 +- tests/unit/cms/classes/AssetTest.php | 18 ++--- tests/unit/cms/classes/CmsObjectTest.php | 4 +- tests/unit/cms/classes/ControllerTest.php | 12 +-- 11 files changed, 241 insertions(+), 80 deletions(-) delete mode 100644 .github/workflows/matchers/phpcs-matcher.json create mode 100755 .github/workflows/utilities/phpcs-pr create mode 100755 .github/workflows/utilities/phpcs-push diff --git a/.github/workflows/code-quality-pr.yaml b/.github/workflows/code-quality-pr.yaml index af05451d1..a63b5013b 100644 --- a/.github/workflows/code-quality-pr.yaml +++ b/.github/workflows/code-quality-pr.yaml @@ -6,18 +6,16 @@ on: jobs: codeQuality: runs-on: ubuntu-latest - name: PHPCS + name: PHP steps: - name: Checkout changes - uses: actions/checkout@v1 + uses: actions/checkout@v2 + with: + fetch-depth: 0 - name: Install PHP and PHP Code Sniffer - uses: shivammathur/setup-php@v1 + uses: shivammathur/setup-php@v2 with: php-version: '7.3' tools: phpcs - - name: Setup problem matcher for PHPCS - run: echo "::add-matcher::${{ github.workspace }}/.github/workflows/matchers/phpcs-matcher.json" - name: Run code quality checks - run: | - git config remote.origin.fetch "+refs/heads/*:refs/remotes/origin/*" && git fetch - phpcs --colors -nq --report="checkstyle" --extensions="php" $(git diff --name-only --diff-filter=ACMR origin/${{ github.base_ref }} HEAD) + run: ./.github/workflows/utilities/phpcs-pr ${{ github.base_ref }} diff --git a/.github/workflows/code-quality-push.yaml b/.github/workflows/code-quality-push.yaml index ff209d0b5..f34f7b3b8 100644 --- a/.github/workflows/code-quality-push.yaml +++ b/.github/workflows/code-quality-push.yaml @@ -9,16 +9,16 @@ on: jobs: codeQuality: runs-on: ubuntu-latest - name: PHPCS + name: PHP steps: - name: Checkout changes - uses: actions/checkout@v1 + uses: actions/checkout@v2 + with: + fetch-depth: 0 - name: Install PHP and PHP Code Sniffer - uses: shivammathur/setup-php@v1 + uses: shivammathur/setup-php@v2 with: php-version: '7.3' tools: phpcs - - name: Setup problem matcher for PHPCS - run: echo "::add-matcher::${{ github.workspace }}/.github/workflows/matchers/phpcs-matcher.json" - name: Run code quality checks - run: phpcs --colors -nq --report="checkstyle" --extensions="php" $(git show --name-only --pretty="" --diff-filter=ACMR ${{ github.sha }}) + run: ./.github/workflows/utilities/phpcs-push ${{ github.sha }} diff --git a/.github/workflows/matchers/phpcs-matcher.json b/.github/workflows/matchers/phpcs-matcher.json deleted file mode 100644 index 5c80b26d2..000000000 --- a/.github/workflows/matchers/phpcs-matcher.json +++ /dev/null @@ -1,23 +0,0 @@ -{ - "problemMatcher": [ - { - "owner": "phpcs", - "severity": "error", - "pattern": [ - { - "regexp": "^$", - "file": 1 - }, - { - "regexp": "+)$", - "line": 1, - "column": 2, - "severity": 3, - "message": 4, - "code": 5, - "loop": true - } - ] - } - ] - } diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index a27246d97..efbd9a2e3 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -13,51 +13,83 @@ jobs: name: JavaScript steps: - name: Checkout changes - uses: actions/checkout@v1 + uses: actions/checkout@v2 + with: + fetch-depth: 0 + - name: Install Node uses: actions/setup-node@v1 with: - node-version: 8 + node-version: 12 + - name: Install Node dependencies run: npm install + - name: Run tests run: npm run test phpUnitTests: - runs-on: ubuntu-latest strategy: max-parallel: 6 matrix: - phpVersions: ['7.2', '7.3', '7.4'] + operatingSystem: [ubuntu-latest, windows-latest] + phpVersion: ['7.2', '7.3', '7.4'] fail-fast: false - name: Unit Tests / PHP ${{ matrix.phpVersions }} + runs-on: ${{ matrix.operatingSystem }} + name: ${{ matrix.operatingSystem }} / PHP ${{ matrix.phpVersion }} + env: + extensions: curl, fileinfo, gd, mbstring, openssl, pdo, pdo_sqlite, sqlite3, xml + key: october-cms-cache-v1 steps: - name: Checkout changes - uses: actions/checkout@v1 + uses: actions/checkout@v2 + + - name: Setup extension cache + id: extcache + uses: shivammathur/cache-extensions@v1 + with: + php-version: ${{ matrix.phpVersion }} + extensions: ${{ env.extensions }} + key: ${{ env.key }} + + - name: Cache extensions + uses: actions/cache@v2 + with: + path: ${{ steps.extcache.outputs.dir }} + key: ${{ steps.extcache.outputs.key }} + restore-keys: ${{ steps.extcache.outputs.key }} + - name: Install PHP - uses: shivammathur/setup-php@v1 + uses: shivammathur/setup-php@v2 with: - php-version: ${{ matrix.phpVersions }} - extensions: mbstring, intl, gd, xml, sqlite - - name: Setup problem matchers for PHPUnit - run: echo "::add-matcher::${{ runner.tool_cache }}/phpunit.json" - - name: Set Composer cache - id: composer-cache + php-version: ${{ matrix.phpVersion }} + tools: composer + extensions: ${{ env.extensions }} + + - name: Setup dependency cache + id: composercache run: echo "::set-output name=dir::$(composer config cache-files-dir)" - - name: Cache Composer dependencies - uses: actions/cache@v1 + + - name: Cache dependencies + uses: actions/cache@v2 with: - path: ${{ steps.composer-cache.outputs.dir }} + path: ${{ steps.composercache.outputs.dir }} key: ${{ runner.os }}-composer-${{ hashFiles('**/composer.json') }} restore-keys: ${{ runner.os }}-composer- + - name: Install Composer dependencies - run: composer install --no-interaction --no-progress --no-suggest --no-scripts + run: composer install --no-interaction --no-progress --no-suggest + - name: Run post-update Composer scripts run: php artisan package:discover + - name: Reset October modules run: | git reset --hard HEAD composer dumpautoload - - name: Run Linting and Tests - run: | - ./vendor/bin/parallel-lint --exclude vendor --exclude storage --exclude tests/fixtures/plugins/testvendor/goto/Plugin.php . - ./vendor/bin/phpunit --prepend ./vendor/october/rain/src/Support/helpers.php + + - name: Setup problem matchers for PHPUnit + if: matrix.phpVersion == '7.4' + run: echo "::add-matcher::${{ runner.tool_cache }}/phpunit.json" + + - name: Run tests + run: ./vendor/bin/phpunit --prepend ./vendor/october/rain/src/Support/helpers.php diff --git a/.github/workflows/utilities/phpcs-pr b/.github/workflows/utilities/phpcs-pr new file mode 100755 index 000000000..48304acc3 --- /dev/null +++ b/.github/workflows/utilities/phpcs-pr @@ -0,0 +1,77 @@ +#!/usr/bin/env php + ($line[3] === 'warning'), + 'message' => $line[4], + 'line' => $line[1], + ]; + } + + // Render report + echo "\e[0;31mFound " + . ((count($lines) === 1) + ? '1 issue' + : count($lines) . ' issues') + . " with code quality.\e[0m"; + echo "\n"; + + foreach ($files as $file => $errors) { + echo "\n"; + echo "\e[1;37m" . str_replace('"', '', $file) . "\e[0m"; + echo "\n\n"; + + foreach ($errors as $error) { + echo "\e[2m" . str_pad(' L' . $error['line'], 7) . " | \e[0m"; + if ($error['warning'] === false) { + echo "\e[0;31mERR:\e[0m "; + } else { + echo "\e[1;33mWARN:\e[0m "; + } + echo $error['message']; + echo "\n"; + } + } + exit(1); +} diff --git a/.github/workflows/utilities/phpcs-push b/.github/workflows/utilities/phpcs-push new file mode 100755 index 000000000..08c9d7fb5 --- /dev/null +++ b/.github/workflows/utilities/phpcs-push @@ -0,0 +1,77 @@ +#!/usr/bin/env php + ($line[3] === 'warning'), + 'message' => $line[4], + 'line' => $line[1], + ]; + } + + // Render report + echo "\e[0;31mFound " + . ((count($lines) === 1) + ? '1 issue' + : count($lines) . ' issues') + . " with code quality.\e[0m"; + echo "\n"; + + foreach ($files as $file => $errors) { + echo "\n"; + echo "\e[1;37m" . str_replace('"', '', $file) . "\e[0m"; + echo "\n\n"; + + foreach ($errors as $error) { + echo "\e[2m" . str_pad(' L' . $error['line'], 7) . " | \e[0m"; + if ($error['warning'] === false) { + echo "\e[0;31mERR:\e[0m "; + } else { + echo "\e[1;33mWARN:\e[0m "; + } + echo $error['message']; + echo "\n"; + } + } + exit(1); +} diff --git a/modules/backend/helpers/Backend.php b/modules/backend/helpers/Backend.php index 18671e94e..de9cae563 100644 --- a/modules/backend/helpers/Backend.php +++ b/modules/backend/helpers/Backend.php @@ -214,14 +214,14 @@ class Backend $contents = file_get_contents($assetFile); // Find all assets that are compiled in this file - preg_match_all('/^=require\s+([A-z0-9-_+\.\/]+)$/m', $contents, $matches, PREG_SET_ORDER); + preg_match_all('/^=require\s+([A-z0-9-_+\.\/]+)[\n|\r\n|$]/m', $contents, $matches, PREG_SET_ORDER); // Determine correct asset path $directory = str_replace(basename($file), '', $file); if (count($matches)) { $results = array_map(function ($match) use ($directory) { - return $directory . $match[1]; + return str_replace('/', DIRECTORY_SEPARATOR, $directory . $match[1]); }, $matches); foreach ($results as $i => $result) { diff --git a/modules/cms/classes/CodeParser.php b/modules/cms/classes/CodeParser.php index b747a20e5..2d1d8ab13 100644 --- a/modules/cms/classes/CodeParser.php +++ b/modules/cms/classes/CodeParser.php @@ -129,7 +129,7 @@ class CodeParser $body = preg_replace('/^\s*function/m', 'public function', $body); $namespaces = []; - $pattern = '/(use\s+[a-z0-9_\\\\]+(\s+as\s+[a-z0-9_]+)?;\n?)/mi'; + $pattern = '/(use\s+[a-z0-9_\\\\]+(\s+as\s+[a-z0-9_]+)?;(\r\n|\n)?)/mi'; preg_match_all($pattern, $body, $namespaces); $body = preg_replace($pattern, '', $body); @@ -142,13 +142,13 @@ class CodeParser foreach ($namespaces[0] as $namespace) { if (str_contains($namespace, '\\')) { - $fileContents .= $namespace; + $fileContents .= trim($namespace).PHP_EOL; } } $fileContents .= 'class '.$className.$parentClass.PHP_EOL; $fileContents .= '{'.PHP_EOL; - $fileContents .= $body.PHP_EOL; + $fileContents .= trim($body).PHP_EOL; $fileContents .= '}'.PHP_EOL; $this->validate($fileContents); diff --git a/tests/unit/cms/classes/AssetTest.php b/tests/unit/cms/classes/AssetTest.php index 13519a161..7325d473e 100644 --- a/tests/unit/cms/classes/AssetTest.php +++ b/tests/unit/cms/classes/AssetTest.php @@ -61,47 +61,47 @@ class AssetTest extends TestCase // Direct paths $this->assertEquals( - $themeDir . '/assets/js/script1.js', + str_replace('/', DIRECTORY_SEPARATOR, $themeDir . '/assets/js/script1.js'), $assetClass->getFilePath('js/script1.js') ); $this->assertEquals( - $themeDir . '/assets/js/script1.js', + str_replace('/', DIRECTORY_SEPARATOR, $themeDir . '/assets/js/script1.js'), $assetClass->getFilePath('/js/script1.js') ); // Direct path to a directory $this->assertEquals( - $themeDir . '/assets/js/subdir', + str_replace('/', DIRECTORY_SEPARATOR, $themeDir . '/assets/js/subdir'), $assetClass->getFilePath('/js/subdir') ); $this->assertEquals( - $themeDir . '/assets/js/subdir', + str_replace('/', DIRECTORY_SEPARATOR, $themeDir . '/assets/js/subdir'), $assetClass->getFilePath('/js/subdir/') ); // Relative paths $this->assertEquals( - $themeDir . '/assets/js/script2.js', + str_replace('/', DIRECTORY_SEPARATOR, $themeDir . '/assets/js/script2.js'), $assetClass->getFilePath('./js/script2.js') ); $this->assertEquals( - $themeDir . '/assets/js/script2.js', + str_replace('/', DIRECTORY_SEPARATOR, $themeDir . '/assets/js/script2.js'), $assetClass->getFilePath('/js/subdir/../script2.js') ); // Missing file, but valid directory (allows for new files) $this->assertEquals( - $themeDir . '/assets/js/missing.js', + str_replace('/', DIRECTORY_SEPARATOR, $themeDir . '/assets/js/missing.js'), $assetClass->getFilePath('/js/missing.js') ); $this->assertEquals( - $themeDir . '/assets/js/missing.js', + str_replace('/', DIRECTORY_SEPARATOR, $themeDir . '/assets/js/missing.js'), $assetClass->getFilePath('js/missing.js') ); // Missing file and missing directory (new directories are created as needed) $this->assertEquals( - $themeDir . '/assets/js/missing/missing.js', + str_replace('/', DIRECTORY_SEPARATOR, $themeDir . '/assets/js/missing/missing.js'), $assetClass->getFilePath('/js/missing/missing.js') ); diff --git a/tests/unit/cms/classes/CmsObjectTest.php b/tests/unit/cms/classes/CmsObjectTest.php index 2c65761b4..895825b5c 100644 --- a/tests/unit/cms/classes/CmsObjectTest.php +++ b/tests/unit/cms/classes/CmsObjectTest.php @@ -23,7 +23,7 @@ class CmsObjectTest extends TestCase $this->assertEquals('

This is a test HTML content file.

', $obj->getContent()); $this->assertEquals('plain.html', $obj->getFileName()); - $path = $theme->getPath().'/testobjects/plain.html'; + $path = str_replace('/', DIRECTORY_SEPARATOR, $theme->getPath().'/testobjects/plain.html'); $this->assertEquals($path, $obj->getFilePath()); $this->assertEquals(filemtime($path), $obj->mtime); } @@ -36,7 +36,7 @@ class CmsObjectTest extends TestCase $this->assertEquals('

This is an object in a subdirectory.

', $obj->getContent()); $this->assertEquals('subdir/obj.html', $obj->getFileName()); - $path = $theme->getPath().'/testobjects/subdir/obj.html'; + $path = str_replace('/', DIRECTORY_SEPARATOR, $theme->getPath().'/testobjects/subdir/obj.html'); $this->assertEquals($path, $obj->getFilePath()); $this->assertEquals(filemtime($path), $obj->mtime); } diff --git a/tests/unit/cms/classes/ControllerTest.php b/tests/unit/cms/classes/ControllerTest.php index 1496055a3..c7860c4a1 100644 --- a/tests/unit/cms/classes/ControllerTest.php +++ b/tests/unit/cms/classes/ControllerTest.php @@ -312,7 +312,7 @@ class ControllerTest extends TestCase ESC; - $this->assertEquals($content, $response); + $this->assertEquals(str_replace(PHP_EOL, "\n", $content), $response); $this->assertEquals(69, $component->property('posts-per-page')); $this->assertEquals('Blog Archive Dummy Component', $details['name']); $this->assertEquals('Displays an archive of blog posts.', $details['description']); @@ -342,7 +342,7 @@ ESC; ESC; - $this->assertEquals($content, $response); + $this->assertEquals(str_replace(PHP_EOL, "\n", $content), $response); $this->assertEquals(6, $component->property('posts-per-page')); $this->assertEquals(9, $component2->property('posts-per-page')); } @@ -403,7 +403,7 @@ ESC; ESC; - $this->assertEquals($content, $response); + $this->assertEquals(str_replace(PHP_EOL, "\n", $content), $response); $this->assertEquals(69, $component->property('posts-per-page')); $this->assertEquals('Blog Archive Dummy Component', $details['name']); $this->assertEquals('Displays an archive of blog posts.', $details['description']); @@ -429,7 +429,7 @@ ESC; ESC; - $this->assertEquals($content, $response); + $this->assertEquals(str_replace(PHP_EOL, "\n", $content), $response); $this->assertEquals(69, $component->property('posts-per-page')); $this->assertEquals('Blog Archive Dummy Component', $details['name']); $this->assertEquals('Displays an archive of blog posts.', $details['description']); @@ -523,7 +523,7 @@ ESC;

Insert post here

ESC; - $this->assertEquals($content, $response); + $this->assertEquals(str_replace(PHP_EOL, "\n", $content), $response); } public function testComponentWithOnRender() @@ -537,6 +537,6 @@ Pass Custom output: Would you look over Picasso's shoulder Custom output: And tell him about his brush strokes? ESC; - $this->assertEquals($content, $response); + $this->assertEquals(str_replace(PHP_EOL, "\n", $content), $response); } } From a6fca1e31769d0ef1689efdede0219aea4e1d775 Mon Sep 17 00:00:00 2001 From: Ben Thomson Date: Fri, 14 Aug 2020 08:26:23 +0800 Subject: [PATCH 2/4] Disable automatic running of scripts in Composer during tests --- .github/workflows/tests.yml | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index efbd9a2e3..9f0ab376b 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -77,7 +77,12 @@ jobs: restore-keys: ${{ runner.os }}-composer- - name: Install Composer dependencies - run: composer install --no-interaction --no-progress --no-suggest + run: composer install --no-interaction --no-progress --no-suggest --no-scripts + + - name: Reset October modules + run: | + git reset --hard HEAD + composer dumpautoload - name: Run post-update Composer scripts run: php artisan package:discover From d98526f6392d7e3d85c9cc4f9c1132e6534514d1 Mon Sep 17 00:00:00 2001 From: Ben Thomson Date: Fri, 14 Aug 2020 10:20:03 +0800 Subject: [PATCH 3/4] Change deprecated PHPUnit calls in MediaLibrary tests --- tests/unit/system/classes/MediaLibraryTest.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/unit/system/classes/MediaLibraryTest.php b/tests/unit/system/classes/MediaLibraryTest.php index 54c4a6a3e..6a281eba8 100644 --- a/tests/unit/system/classes/MediaLibraryTest.php +++ b/tests/unit/system/classes/MediaLibraryTest.php @@ -78,10 +78,10 @@ class MediaLibraryTest extends TestCase // @codingStandardsIgnoreLine $this->assertNotEmpty($contents, 'Media library item is not discovered'); $item = reset($contents); - $this->assertAttributeEquals('file', 'type', $item, 'Media library item does not have the right type'); - $this->assertAttributeEquals('/text.txt', 'path', $item, 'Media library item does not have the right path'); - $this->assertAttributeNotEmpty('lastModified', $item, 'Media library item last modified is empty'); - $this->assertAttributeNotEmpty('size', $item, 'Media library item size is empty'); + $this->assertEquals('file', $item->type, 'Media library item does not have the right type'); + $this->assertEquals('/text.txt', $item->path, 'Media library item does not have the right path'); + $this->assertNotEmpty($item->lastModified, 'Media library item last modified is empty'); + $this->assertNotEmpty($item->size, 'Media library item size is empty'); } protected function setUpStorage() From d52893dd43766241e065c298b2a2e83db6c7bedf Mon Sep 17 00:00:00 2001 From: Ben Thomson Date: Fri, 14 Aug 2020 16:10:05 +0800 Subject: [PATCH 4/4] Will need to use PluginTestCase for FileModel to be supported --- tests/unit/system/classes/ImageResizerTest.php | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/unit/system/classes/ImageResizerTest.php b/tests/unit/system/classes/ImageResizerTest.php index 4590ee153..239d0ac90 100644 --- a/tests/unit/system/classes/ImageResizerTest.php +++ b/tests/unit/system/classes/ImageResizerTest.php @@ -1,13 +1,11 @@