Skip to content

Commit

Permalink
Allow CRLF files in /tests/fixtures/ directories
Browse files Browse the repository at this point in the history
In moodlehq#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 moodlehq#203
  • Loading branch information
stronk7 committed Sep 8, 2022
1 parent 4da2977 commit fa8545d
Show file tree
Hide file tree
Showing 6 changed files with 48 additions and 13 deletions.
8 changes: 6 additions & 2 deletions locallib.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
6 changes: 3 additions & 3 deletions tests/behat/ui.feature
Original file line number Diff line number Diff line change
Expand Up @@ -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! |
Expand All @@ -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 |
Expand Down
13 changes: 6 additions & 7 deletions tests/coverage.php
Original file line number Diff line number Diff line change
Expand Up @@ -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. */
Expand Down
3 changes: 3 additions & 0 deletions tests/fixtures/crlf.csv
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
f1,f2,f3
1,2,3
4,5,6
3 changes: 3 additions & 0 deletions tests/fixtures2/crlf.csv
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
f1,f2,f3
1,2,3
4,5,6
28 changes: 27 additions & 1 deletion tests/locallib_test.php
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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('<xml/>');
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('<xml/>');
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('<xml/>');
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());
}
}

0 comments on commit fa8545d

Please sign in to comment.