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

Keeping the test-utils class while removing the tests. #3158

Open
VincentLanglet opened this issue Nov 4, 2020 · 4 comments
Open

Keeping the test-utils class while removing the tests. #3158

VincentLanglet opened this issue Nov 4, 2020 · 4 comments
Milestone

Comments

@VincentLanglet
Copy link
Contributor

I saw multiple issue about adding or removing the tests from the gitattributes
#548
#1908

Imho there is two king of files in the tests/ dirs

  • Tests which are used to test a specific sniff/file
  • Utils files which are used to run the test

I think the best would be to exclude the first ones but keep the second one (for developper which use them to run test

For instance, it's right to export-ignore this in the next major.

/src/Standards/Generic/Tests export-ignore
/src/Standards/MySource/Tests export-ignore
/src/Standards/PEAR/Tests export-ignore
/src/Standards/PSR1/Tests export-ignore
/src/Standards/PSR2/Tests export-ignore
/src/Standards/PSR12/Tests export-ignore
/src/Standards/Squiz/Tests export-ignore
/src/Standards/Zend/Tests export-ignore

But I'm more concern about this line

/tests export-ignore

When we're writing on our sniff,
The class tests\Standards\AbstractMethodUnitTest is useful.
Same for tests\bootstrap.php and maybe others (and all the dependencies).

Maybe there's something to do about it, like not excluding everything or moving some files somewhere else.

@gsherwood
Copy link
Member

As /tests still contains quite a lot of test code in .inc files, what if I instead excluded /tests/Core and left the rest of the /tests directory alone. Would that work?

@gsherwood gsherwood added this to the 4.0.0 milestone Nov 9, 2020
@VincentLanglet
Copy link
Contributor Author

As /tests still contains quite a lot of test code in .inc files, what if I instead excluded /tests/Core and left the rest of the /tests directory alone. Would that work?

This seems like a great solution indeed.

The only files which won't work is the test/AllTest.php file, since it require 'Core/AllTests.php'

require_once 'Core/AllTests.php';
require_once 'Standards/AllSniffs.php';
require_once 'TestSuite.php';

class PHP_CodeSniffer_AllTests
{
    public static function suite()
    {
        $GLOBALS['PHP_CODESNIFFER_STANDARD_DIRS'] = [];
        $GLOBALS['PHP_CODESNIFFER_TEST_DIRS']     = [];

        $suite = new TestSuite('PHP CodeSniffer');

        $suite->addTest(Core\AllTests::suite());
        $suite->addTest(Standards\AllSniffs::suite());

        return $suite;

    }
}

Don't know if this file should be excluded or instead modified to check for the existence of Core\AllTests before adding the suite.

@gsherwood
Copy link
Member

Don't know if this file should be excluded or instead modified to check for the existence of Core\AllTests before adding the suite.

That was my thought too. I figure checking for it before including it might be the way to go here.

@jlherren
Copy link

I welcome this change very much. To add my 2 cents: Everything that will remain in the composer package after this change must necessarily be namespaced.

I'm currently running into an issue where my IDE is showing wrong quick help for gettype():

image

Note the wrong return type string|void and the missing PhpDoc text. The reason for this is the function getType() defined in Tests\PHP\LowercasePHPFunctionsUnitText.inc. (And yes, I can of course manually exclude directories from indexing, but it would be nicer if I didn't have to.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants