Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tests for coding handling STDIN for the sniffs FileName and I18nTextDomainFixer #2512

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

rodrigoprimo
Copy link
Collaborator

This PR adds a test to cover code handling STDIN for the two sniffs that check it: WordPress.Files.FileName and WordPress.Utils.I18nTextDomainFixer. This is based on a related suggestion from @jrfnl for a PHPCS sniff: PHPCSStandards/PHP_CodeSniffer#681 (review).

I'm not sure if I missed something, but I could not make the ConfigDouble class work passing the standard and sniff names as parameters. I had to create a custom ruleset file and include the sniff passing its path. I believe this happens because the ConfigDouble class erases the config data, and thus, installed_paths is empty.

The sniff bails early when handling STDIN. This commit adds a test to cover that part of the sniff code.
The sniff bails early when handling a plugin header passed via STDIN. This commit adds a test to cover that part of the sniff code.
@jrfnl
Copy link
Member

jrfnl commented Nov 30, 2024

He @rodrigoprimo, thanks for this PR!

I'm not sure if I missed something, but I could not make the ConfigDouble class work passing the standard and sniff names as parameters. I believe this happens because the ConfigDouble class erases the config data, and thus, installed_paths is empty.

The ConfigDouble doesn't "erase" the config data, otherwise nothing would work. However, it does skip reading the CodeSniffer.conf file, which is where the installed_paths is stored.

Reason for that is threefold:

  1. Performance. Reading the conf file every time a Config needs to be created in the tests has an impact on performance.
  2. The user-specific CodeSniffer.conf file may also contain things like global colors or report_width overrides, which can break test presumptions.
  3. Changing any settings which are be stored in the conf file on a Config in the tests will affect static variables in the Config class, which can then affect (and break) tests being run after the test making these changes.
    The ConfigDouble still allows for making those changes, but will reset the static properties on the Config class when the test has finished to prevent that.

Does that help your understanding of the ConfigDouble ?

I had to create a custom ruleset file and include the sniff passing its path.

Nice solution, though I do think there are alternative solutions which don't require the extra file.

Alternative 1: use the real Config, not the ConfigDouble.

use PHP_CodeSniffer\Config;

		$config            = new Config();
		$config->standards = ['WordPress'];
		$config->sniffs    = ['WordPress.Files.Filename'];

Not great for performance of the tests, but should work.

Alternative 2: use the PHPCSUtils 1.1.0 ConfigDouble (yes, I do still need to tag that).

use PHPCSUtils\BackCompat\Helper;
use PHPCSUtils\TestUtils\ConfigDouble;

		$config            = new ConfigDouble();
		Helper::setConfigData('installed_paths', dirname(dirname(__DIR__)), true, $config);
		$config->standards = ['WordPress'];
		$config->sniffs    = ['WordPress.Files.Filename'];

What do you think of those possibilities ?

@rodrigoprimo
Copy link
Collaborator Author

Thanks for all the information and the PR review, @jrfnl!

Alternative 1: use the real Config, not the ConfigDouble.
Not great for performance of the tests, but should work.

I can confirm that alternative 1 works indeed, as I tested it while trying to figure out why doing the same with ConfigDouble doesn't work. I discarded it due as it is mentioned in the ConfigDouble docblock that it should be used in most cases instead of Config when creating tests.

Alternative 2: use the PHPCSUtils 1.1.0 ConfigDouble (yes, I do still need to tag that).

This seems to be the best solution, and I don't see a problem with waiting until PHPCSUtils 1.1.0 is released to finish this PR. How does that sound?

@jrfnl
Copy link
Member

jrfnl commented Dec 2, 2024

Alternative 2: use the PHPCSUtils 1.1.0 ConfigDouble (yes, I do still need to tag that).

This seems to be the best solution, and I don't see a problem with waiting until PHPCSUtils 1.1.0 is released to finish this PR. How does that sound?

Works for me 👍

@jrfnl jrfnl added Status: Requirement not met On hold until WPCS supports a particular version of a dependency (PHP, PHP_CodeSniffer, etc.). Upstream: PHPCSUtils labels Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Requirement not met On hold until WPCS supports a particular version of a dependency (PHP, PHP_CodeSniffer, etc.). Type: Chores/Cleanup Upstream: PHPCSUtils
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants