Php_codesniffer: Remove tests from the composer package

[neomerx]

Firstly, thank you for this tool.

Could you please exclude test classes/methods/ etc from the release and use dedicated namespaces instead of the global one?

IMHO the nastiest are these ones as they constantly show up in IDE intellisense.

[jrfnl]

See #548

[neomerx]

@Craige

the tests/ directory is very valuable

without explaining why however later he mentioned he needed them because he was developing some coding standard

I'm really only concerning myself with development of this standard as this use-case.

As far as I understand, having those tests included serves uncommon use case and punishes all regular users who are OK with included standards.

Also, the necessity of having those tests do not justify polluting the global namespace.

[gsherwood]

Every once and a while, someone asks for the tests directory to be excluded. If I do that, someone asks for the tests directory to be included. I can't win.

So I'm leaving the tests directory in because it reflects the full code base of PHPCS. If there comes a time (or someone knows how right now) when composer can be told to include the tests directory, then I'll exclude it by default and sniff developers can selectively include it.

I'll leave this open as a request to add namespace lines to all test files, and adjust all error message lines accordingly. But I have to admit that this is a low priority for me given the mountain of other requests I have, so I'd be relying on someone else to submit a PR for it.

[jrfnl]

If there comes a time (or someone knows how right now) when composer can be told to include the tests directory, then I'll exclude it by default and sniff developers can selectively include it.

Luckily, that is already available ;-)

The default for Composer is --prefer-dist and if a .gitattributes file would be added with export-ignores for the test directory and other dev related files, those wouldn't be included anymore by default.

For people who develop for PHPCS, all they'd need to do is run Composer with --prefer-source and they'd get the complete package, including tests etc.

[gsherwood]

For people who develop for PHPCS, all they'd need to do is run Composer with --prefer-source and they'd get the complete package, including tests etc.

I know about that option, but I was told previously that this is not how people wanted to install PHPCS, even with the tests dir. I think it was something about caching. I'd be happy to create two version of PHPCS (with and without tests) if there was some way of installing the one you want.

[neomerx]

@gsherwood just to illustrate you why I'm here with the issue

when I need to compare a variable with null and type nu IDE shows the following

this first selection of null() comes from this. Seeing it many times every day is frustrating.

Any other options to solve the issue rather than remove the library from dependencies?

[neomerx]

Every once and a while, someone asks for the tests directory to be excluded.

The key question would be 'who is that someone?' if you expect your package to be primarily used by new code styles developers then yes that's a right choice to leave the tests, but if 99% of them use built-in styles then probably not IMHO.

[jrfnl]

I know about that option, but I was told previously that this is not how people wanted to install PHPCS, even with the tests dir. I think it was something about caching.

I'm with @neomerx here. If you develop sniffs, you're better off working off a git clone anyway to allow you to quickly checkout various PHPCS versions and run tests against them.

Shipping without tests will diminish the size of the package significantly and benefit 99% of users of PHPCS.

[neomerx]

Possible solutions to date for those who experience the issue

• stick to 2.9 version, do not upgrade and wait for the issue fix
• use the latest version and manually remove multiple Tests folders after every update
• remove php_codesniffer from composer.json and wait for the issue fix

[Craige]

I know about that option, but I was told previously that this is not how people wanted to install PHPCS, even with the tests dir. I think it was something about caching. I'd be happy to create two version of PHPCS (with and without tests) if there was some way of installing the one you want.

@gsherwood - Just spit balling here, but what about putting the tests in a separate repository, and adding it to the require-dev object? The documentation suggests this could be a reasonable solution, as require-dev is explicitly for...

packages required for developing this package, or running tests, etc

E.g:

# on master
{
    "name": "squizlabs/php_codesniffer",
    "require-dev": {
        "squizlabs/php_codesniffer-tests": "master-dev"
    }
}

# on 3.0
{
    "name": "squizlabs/php_codesniffer",
    "require-dev": {
        "squizlabs/php_codesniffer-tests": "3.0-dev"
    }
}

Outside the PHP world, it seems to be more common to put your unit tests in a separate repository, but in PHP they seem to be included. I think this is a middle ground that keeps the separate, but still linked.

[gsherwood]

Just spit balling here, but what about putting the tests in a separate repository

I don't use composer, so that would make my life really hard. Plus I'd need to change the way the tests are being discovered and run, which is painful, but also a huge BC break for custom standards.

Thanks for the idea though.

[tomasfejfar]

Just my 2¢:

I understand the logic behind including tests. I also understand the motivation for sniff developers.

Developing sniffs (which is what tests are useful for) is quite complex. I think it's safe to assume that it's something only somewhat advanced developers do. Such developers are usually more comfortable with their tools. Therefore are able to checkout using --prefer-dist, change the timeout in composer in case it takes too much time and generally just handle the raised complexity. Also those developers may be more comfortable with opensource workflow and may be more likely to submit an issue.

On the other hand, the less advanced users tend to be less comfortable with their tools (IDE included). So they may not know how to get rid of the global namespace pollution (hint: PhpStorm can exclude one specific directory - "Mark directory as"→"Excluded"), may not be comfortable with additional files that appear in their file search (hint: You can create a Scope that excludes the vendor directory), etc. They are also less likely to submit an issue either because they think it's their mistake or are not used to create issues in general. So for them having phpcs without tests would be much better.

So tu sum up, the current solution IMHO complicates life for less advanced users and makes it harder for them to justify using phpcs and makes life easier for a relatively smaller group of more advanced users that would have been able to find and use a workaround. I'd argue that removing the tests is the right thing to do. I also think that #548 did not propose significant use case, escpecially if there is a a workaround available.

[gsherwood]

I'm not going to remove the tests from the 3.x version. It people are relying on them being there, and have been relying on them being there for over a year, removing them in a point release seems like a shitty thing to do.

But I will add this to the 4.0.0 roadmap and have them removed as part of that version unless someone else comes up with another good reason why they should stay.

[tomasfejfar]

That seems like a sensible aproach. Thanks.

[PatrickRose]

What about people who aren't developing a separate library and are using PHPCS? If you're a sniff developer then I can kind of get the argument but I think that's an unnecessary step for someone to take. It also means it's _really_ hard to run sniff tests in a CI environment. For our purposes, I'll have to possibly extract our sniffs into a separate library and write a new CI job as well. Maybe that's something we should just do anyway, but it'd be a minor pain for us to do so.

Btw, the change I've done in the past to prevent this causing an issue for us was to rename the files to be .php.1 so they didn't get parsed as PHP but PHPCS could still find them. Maybe that's the solution here instead of namespacing them because that'll be cleaner (and can be done in 3.*)?

[gsherwood]

Test files have now been removed again, for version 4.0

[neomerx]

Yay! 2 years of waiting and I can upgrade, finally :fireworks: