From fa8545da210cda2d4c3ae5551df0f25307117900 Mon Sep 17 00:00:00 2001 From: "Eloy Lafuente (stronk7)" Date: Fri, 9 Sep 2022 00:01:00 +0200 Subject: [PATCH 1/2] Allow CRLF files in /tests/fixtures/ directories In #203 it was proposed to allow CRLF files in Moodle, because the original CSV specs say that those files should be CRLF. But in practice, nowadays hardly any file is CRLF and, specifically for Moodle, we have banned them in core since the night of the times. And we should continue not allowing them. Hence we are only allowing those CRLF CSV files within the /tests/fixtures/ directories, that way if anybody wants to test their CSV loaders with CRLF, they can have some fixtures at hand. Everywhere else, they will continue being forbidden. Fixes #203 --- locallib.php | 8 ++++++-- tests/behat/ui.feature | 6 +++--- tests/coverage.php | 13 ++++++------- tests/fixtures/crlf.csv | 3 +++ tests/fixtures2/crlf.csv | 3 +++ tests/locallib_test.php | 28 +++++++++++++++++++++++++++- 6 files changed, 48 insertions(+), 13 deletions(-) create mode 100644 tests/fixtures/crlf.csv create mode 100644 tests/fixtures2/crlf.csv diff --git a/locallib.php b/locallib.php index a1125eff..0690cb86 100644 --- a/locallib.php +++ b/locallib.php @@ -333,15 +333,19 @@ function local_codechecker_check_other_file($file, $xml) { // Certain files are permitted lines of any length because they are // Auto-generated. $allowanylength = in_array(basename($file), array('install.xml')) || - substr($file, -4, 4) === '.csv'; + substr($file, -4, 4) === '.csv'; + // We allow CRLF line endings in .csv tests/fixtures files (see #203). + $allowcrlf = substr($file, -4, 4) === '.csv' && + strpos($file, '/tests/fixtures/') !== false; $lines = file($file); $index = 0; $blankrun = 0; + $donecrlf = false; foreach ($lines as $l) { $index++; // Incorrect [Windows] line ending. - if ((strpos($l, "\r\n") !== false) && empty($donecrlf)) { + if (!$donecrlf && !$allowcrlf && (strpos($l, "\r\n") !== false)) { local_codechecker_add_problem($fileinxml, $file, $index, 'crlf'); $donecrlf = true; } diff --git a/tests/behat/ui.feature b/tests/behat/ui.feature index 946eb22f..b71abd1b 100644 --- a/tests/behat/ui.feature +++ b/tests/behat/ui.feature @@ -19,7 +19,7 @@ Feature: Codechecker UI works as expected | index2.php | Invalid path index2.php | Files found: 1 | | local/codechecker/version.php | Well done! | Invalid path | | local/codechecker/tests/ | checker/tests/locallib_test.php | Invalid path | - | local/codechecker/tests/ | Files found: 9 | Invalid path | + | local/codechecker/tests/ | Files found: 11 | Invalid path | | local/codechecker/tests/locallib_test.php | Well done! | Invalid path | | local/codechecker/tests/fixtures/behat/problem.php | Files found: 1 | Invalid path | | local/codechecker/tests/fixtures/behat/problem.php | Total: 2 error(s) and 1 warning(s) | Well done! | @@ -38,8 +38,8 @@ Feature: Codechecker UI works as expected Examples: | path | exclude | seen | notseen | - | local/codechecker/tests | */tests/fixtures/* | Files found: 2 | Invalid path | - | local/codechecker/tests/ | *one*, *moodle_* | Files found: 8 | Invalid path | + | local/codechecker/tests | */tests/fixtures/* | Files found: 3 | Invalid path | + | local/codechecker/tests/ | *one*, *moodle_* | Files found: 10 | Invalid path | | local/codechecker/tests | */tests/fixtures/* | locallib_test.php | problem.php | | local/codechecker/tests/ | *moodle_* | Line 1 of the opening comment | moodle_php | | local/codechecker/tests/ | *moodle_* | fixtures/behat/phpcompat | /moodle_php | diff --git a/tests/coverage.php b/tests/coverage.php index 35b1697d..a306d686 100644 --- a/tests/coverage.php +++ b/tests/coverage.php @@ -30,18 +30,17 @@ */ return new class extends phpunit_coverage_info { /** @var array The list of folders relative to the plugin root to include in coverage generation. */ - protected $includelistfolders = [ - 'classes', - 'moodle', - ]; + protected $includelistfolders = []; /** @var array The list of files relative to the plugin root to include in coverage generation. */ - protected $includelistfiles = [ - 'locallib.php']; + protected $includelistfiles = []; /** @var array The list of folders relative to the plugin root to exclude from coverage generation. */ protected $excludelistfolders = [ - 'moodle/tests', + 'MoodleCS', + 'moodle', + 'PHPCompatibility', + 'phpcs', ]; /** @var array The list of files relative to the plugin root to exclude from coverage generation. */ diff --git a/tests/fixtures/crlf.csv b/tests/fixtures/crlf.csv new file mode 100644 index 00000000..862d3d42 --- /dev/null +++ b/tests/fixtures/crlf.csv @@ -0,0 +1,3 @@ +f1,f2,f3 +1,2,3 +4,5,6 diff --git a/tests/fixtures2/crlf.csv b/tests/fixtures2/crlf.csv new file mode 100644 index 00000000..862d3d42 --- /dev/null +++ b/tests/fixtures2/crlf.csv @@ -0,0 +1,3 @@ +f1,f2,f3 +1,2,3 +4,5,6 diff --git a/tests/locallib_test.php b/tests/locallib_test.php index b28f7167..0e6415cf 100644 --- a/tests/locallib_test.php +++ b/tests/locallib_test.php @@ -108,7 +108,7 @@ public function local_codechecker_find_other_files_provider() { * @param string[] $nomatches list of substring-matching strings not expected to be in the results. * * @dataProvider local_codechecker_find_other_files_provider() - * @covers ::local_codechecker_find_other_files() + * @covers ::local_codechecker_find_other_files */ public function test_local_codechecker_find_other_files(string $path, array $ignores, array $extensions, array $matches, array $nomatches) { @@ -143,4 +143,30 @@ public function test_local_codechecker_find_other_files(string $path, array $ign $this->assertStringNotContainsString($nomatch, $results); } } + + /** + * Verify test_local_codechecker_check_other_file() behaviour. + * + * @covers ::local_codechecker_check_other_file + */ + public function test_local_codechecker_check_other_file() { + + require_once(__DIR__ . '/../locallib.php'); + + // Verify that lf files are ok. + $xml = new \SimpleXMLElement(''); + local_codechecker_check_other_file(__DIR__ . '/../version.php', $xml); + $this->assertStringContainsString('errors="0" warnings="0"', $xml->asXML()); + + // Verify crlf files in /tests/fixtures/ locations are ok. + $xml = new \SimpleXMLElement(''); + local_codechecker_check_other_file(__DIR__ . '/fixtures/crlf.csv', $xml); + $this->assertStringContainsString('errors="0" warnings="0"', $xml->asXML()); + + // Verify crlf files in not in /tests/fixtures/ locations are wrong. + $xml = new \SimpleXMLElement(''); + local_codechecker_check_other_file(__DIR__ . '/fixtures2/crlf.csv', $xml); + $this->assertStringContainsString('errors="1" warnings="0"', $xml->asXML()); + $this->assertStringContainsString('Windows (CRLF) line ending instead of just LF', $xml->asXML()); + } } From 3c7a20b712382d562650f77ea6bbbf78fd6789ed Mon Sep 17 00:00:00 2001 From: "Eloy Lafuente (stronk7)" Date: Fri, 9 Sep 2022 00:05:58 +0200 Subject: [PATCH 2/2] Bump GHA environments to use Ubuntu 22.04 (up from 18.04) Since some time ago, GH is warning us that the Ubuntu 18.04 environment is deprecated: "The ubuntu-18.04 environment is deprecated, consider switching to ubuntu-20.04(ubuntu-latest), or ubuntu-22.04 instead...." Hence, we are bumping here, to be able to cerify if everything continues working ok --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index be4ae227..310c0350 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -4,7 +4,7 @@ on: [push, pull_request] jobs: test: - runs-on: ubuntu-18.04 + runs-on: ubuntu-22.04 services: postgres: