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

Generic/LowercasedFilename: improve code coverage #681

Merged

Conversation

rodrigoprimo
Copy link
Contributor

@rodrigoprimo rodrigoprimo commented Nov 13, 2024

Description

This PR improves code coverage for the Generic.Files.LowercasedFilename sniff. I had to create a test file that does not follow the naming convention used by PHPCS to test how the sniff behaves when handling a file that only contains lowercase letters. To improve the readability of this file, I also used underscores.

Related issues/external references

Part of #146
Issue found while working on this PR: #682

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.

Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Happy to accept this PR once the ignore annotation has been removed.

I have two "pro-tips" related to this PR to keep in mind for the future:

  1. When test case files can't comply with the naming conventions
    For this PR only one extra file which couldn't comply with the test case file naming convention needed to be added. In which case, adding the file the way it is done in this PR is perfectly fine.
    However, if you run across a sniff in the future, which needs multiple extra test case files which can't follow the SniffNameUnitTest.#.inc test case file name convention, things can get messy.
    So, my pro-tip for that case would be to create a sub-directory in the relevant test directory based on the sniff name and put all the additional test case files in that subdirectory.
    In the test file getTestFiles() method, you can then use a call to glob() to get a file list to return to the test runner.
    Unfortunately, I couldn't find an example of such a setup in this GH org, but you can see an example of how this pattern can be applied in the WPCS standard for their FileName sniff.
    Hope that helps.
  2. Testing STDIN
    It is perfectly possible to test code handling STDIN, so code blocks related to that should never be ignored for code coverage.
    Yes, it does not fit within the "normal" testing framework for testing sniffs, but that doesn't mean, it can't be done.
    What would be needed for this, is to introduce a new test method to the test file. This may not be common, but there is nothing technically blocking this and the DummyFile class can be used to pass the input code to such tests.
    The new test method would then look something like this:
    use PHP_CodeSniffer\Files\DummyFile;
    use PHP_CodeSniffer\Ruleset;
    use PHP_CodeSniffer\Tests\ConfigDouble;
    
    /*...*/
    
    /**
     * Test stdin.
     *
     * @return void
     */
    public function testStdIn()
    {
        $config            = new ConfigDouble();
        $config->standards = ['Generic'];
        $config->sniffs    = ['Generic.Files.LowercasedFilename'];
    
        $ruleset = new Ruleset($config);
    
        $content = '<?php '.PHP_EOL;
        $file    = new DummyFile($content, $ruleset, $config);
        $file->process();
    
        $this->assertSame(0, $file->getErrorCount());
        $this->assertSame(0, $file->getWarningCount());
        $this->assertCount(0, $file->getErrors());
    
    }//end testStdIn()
    And such a test could even be set up with a data provider to test multiple different code snippets and expectations.
    For some examples of test methods using a similar pattern, have a look at the ErrorSuppressionTest.

@rodrigoprimo
Copy link
Contributor Author

When test case files can't comply with the naming conventions

Thanks. I will keep this in mind if I need to create tests with multiple files that don't follow the naming convention.

Testing STDIN

TIL! Thanks!

As I mentioned in the inline comment, I removed the annotation and added the test that you suggested.

This PR is ready for another review.

@rodrigoprimo
Copy link
Contributor Author

I forgot to mention in the previous comment that when implementing the test, I removed PHP_EOL from the contents of the dummy file as it seems it is not necessary. I am highlighting this here in case I missed something, and there is a reason to keep PHP_EOL.

@jrfnl
Copy link
Member

jrfnl commented Nov 26, 2024

I forgot to mention in the previous comment that when implementing the test, I removed PHP_EOL from the contents of the dummy file as it seems it is not necessary. I am highlighting this here in case I missed something, and there is a reason to keep PHP_EOL.

All good, I just posted a rough example. Presumed you would customize it to what was needed.

Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @rodrigoprimo!

@jrfnl jrfnl added this to the 3.11.2 milestone Nov 26, 2024
@jrfnl jrfnl merged commit 609ac10 into PHPCSStandards:master Nov 26, 2024
58 checks passed
@jrfnl jrfnl deleted the test-coverage-lowercased-filename branch November 26, 2024 03:16
jrfnl pushed a commit that referenced this pull request Nov 26, 2024
Includes adding a test for handling STDIN per suggestion during code review.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants