Filters/Filter: expand test coverage for the Filter::accept() method

[rodrigoprimo]

Description

This PR expands the Filter::accept() tests to cover cases that were not covered before:

• Run the tests on Windows as there is dedicated code when the directory separator is "\".
• Real directories trigger code that relies on is_dir() returning true.
• Sets Config::$local to true and ensure in this case directories are not accepted.
• Files without an extension, with an invalid one or starting with a dot are ignored.
• Duplicated files are ignored.
• Relative exclude patterns are handled correctly.

Suggested changelog entry

N/A

Additional notes

There is still one uncovered if block in a protected method called by Filter::accept():

PHP_CodeSniffer/src/Filters/Filter.php

Lines 250 to 255 in d140424

	// Maintains backwards compatibility in case the ignore pattern does
	// not have a relative/absolute value.
	if (is_int($pattern) === true) {
	$pattern = $type;
	$type = 'absolute';
	}

As far as I can check, there is no need to maintain this backwards compatibility code anymore, and it can be removed. It was added via squizlabs/PHP_CodeSniffer@4982619 to preserve backwards compatibility back in a time when it was possible to programmatically set ignore patterns using CodeSniffer::setIgnorePatterns() (see https://pear.php.net/bugs/bug.php?id=19859). This method was removed a long time ago via f61025c#diff-c36ecedca179eab0b3cd245e872a96ab26fa08e567437e167f4eda1779c15c89L431-L435, and since then, I don't think there is a way for users to set the ignore pattern array with numeric indices. Happy to open a separate issue to discuss this if you think that is a good idea.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
  • This change is only breaking for integrators, not for external standards or end-users.
• Documentation improvement

PR checklist

• I have checked there is no other PR open for the same change.
• I have read the Contribution Guidelines.
• I grant the project the right to include and distribute the code under the BSD-3-Clause license (and I have the right to grant these rights).
• I have added tests to cover my changes.
• I have verified that the code complies with the projects coding standards.
• [Required for new sniffs] I have added XML documentation for the sniff.

[jrfnl]

@rodrigoprimo Thanks for this PR! Good finds, but this needs more work.

Primary remark: you have added all new data sets to the testExcludePatterns() test, while most of them have nothing to do with testing exclude patterns.
This is confusing. Either that test method needs to be renamed or the tests should be split up (split up is preferred).

Also have a look at the initializeConfigAndRuleset() (setUpBeforeClass()) method which contains specific settings (standard + ignore pattern) related to the pre-existing tests.
These will now influence the new tests too, which makes those impure.

Lastly, keep in mind that - as per the test class name - the tests in that file are primarily for the Filter::accept() method, so tests/data sets testing other parts of the Filter class should probably get their own test class to keep the tests maintainable.

Additional notes:

• PHPCS 4.0 has a dedicated ShouldProcessFileWithoutExtensionTest class for testing handling of files without extension.
  The "Filter should exclude files without an extension, using unsupported extension and starting with a dot" data set should probably be split into three data sets, each testing only one thing (1. without extension, 2. unsupported extension and 3. starting with a dot).
  This would also make it more straight-forward to merge this up into PHPCS 4.x (and remove the test duplication).
• The "Unsupported extension" test should probably have its own test method (or maybe even test class) and should test the same set of input paths with different Config extensions settings.
  The current test is just confirming the default behaviour, not testing that the extensions setting is respected.
  The test should probably should also be marked explicitly as covering the Filter::shouldProcessFile() method.
• The test related to "local" files should probably have its own test method (or maybe even test class) and test with explicit true/false settings.
  You may also want to have a look at issue #1067 which is related to this (but to be addressed in a separate PR).
• The test related to the ruleset type attribute (exclude-pattern type="relative") should get a sister-test with type="absolute". It should also get a test where the relative path is not matched. You're now just testing the happy path.

While this PR improves the tests, it also ends up hiding the fact that the tests are still far from complete as code coverage will show as higher, while in reality, there is still a lot which isn't properly tested.

You may also want to consider using test fixtures to test the directory handling to make the tests more stable.

I don't think there is a way for users to set the ignore pattern array with numeric indices. Happy to open a separate issue to discuss this if you think that is a good idea.

Agreed and thanks for checking the history. I don't think an issue is needed. A PR to remove that outdated code will be fine (with your above analysis of why it can be removed in the commit messages).

[rodrigoprimo]

As discussed with @jrfnl during a call, I'm closing this PR, and I will revisit expanding test coverage for this method another time if no one gets to it before I do.