Test organisation

[jrfnl]

Just a question.

The test file is quite large containing lots of test functions.

Is there any particular reason why this hasn't been split up to, for instance, one test file for each test case file ?

[sirbrillig]

No particular reason. It just grew from humble origins. 😁

[jrfnl]

That's fair enough.

I just noticed it as for some things it is quite unclear to me which file to add tests to, so for now, I've just elected to add completely new test case files.

[jrfnl]

I'm looking at this now for the next iteration on the tests.

If each test case file has it's own test file, the fixture file name could be moved to a class constant and the BaseTestCase::prepareLocalFileForSniffs() method could just get the fixture directly using static::FIXTURE_FILE.
That would remove the second parameter of the method as well.
And we could even move the call to $phpcsFile->process() to the prepareLocalFileForSniffs()method too as that's needed for every test too, or maybe better move most of this to a setUp() method or even a setUpBeforeClass().
It would simplify the code in the actual tests a lot.

How would you feel about that ?

[sirbrillig]

These are great ideas, thank you! 🤔 I definitely like the idea of simplifying tests.

I don't think we can easily move $phpcsFile->process() to the setup or prepare functions because if we need to call setSniffProperty() we have to do it on the File object before the processing occurs. Maybe we could do something like add a separate "prepare" method that accepts arguments to change the sniff properties... The only thing that I want to avoid is adding too much implicit code over explicit code.

Still, the idea of moving each test case into its own class to take advantage of pairing fixtures is an interesting one.

Let's take a look at some examples. Here's a pretty typical existing test:

  public function testFunctionWithDefaultParamWarnings() {
    $fixtureFile = $this->getFixture('FunctionWithDefaultParamFixture.php');
    $phpcsFile = $this->prepareLocalFileForSniffs($fixtureFile);
    $phpcsFile->ruleset->setSniffProperty(
      'VariableAnalysis\Sniffs\CodeAnalysis\VariableAnalysisSniff',
      'allowUnusedParametersBeforeUsed',
      'false'
    );
    $phpcsFile->process();
    $lines = $this->getWarningLineNumbersFromFile($phpcsFile);
    $expectedWarnings = [
      3,
      14,
    ];
    $this->assertEquals($expectedWarnings, $lines);
  }

It could be moved to its own class with a static fixture name and changed to:

  public function testFunctionWithDefaultParamWarnings() {
    $phpcsFile = $this->prepareLocalFileForSniffs();
    $phpcsFile->ruleset->setSniffProperty(
      'VariableAnalysis\Sniffs\CodeAnalysis\VariableAnalysisSniff',
      'allowUnusedParametersBeforeUsed',
      'false'
    );
    $phpcsFile->process();
    $lines = $this->getWarningLineNumbersFromFile($phpcsFile);
    $expectedWarnings = [
      3,
      14,
    ];
    $this->assertEquals($expectedWarnings, $lines);
  }

That's one less line, which does add up in a large number of tests, but it's not a huge savings with the 30 different fixtures we have now. It would be nice to not have one monolithic test file, though!

[jrfnl]

Alternatively, you could consider switching over to the PHPCS native AbstractSniffUnitTest class as a base.

• That would still leave one main test class with a getErrors() and a getWarnings() method with the lines numbers where issues are expected + the count for each line for each fixture file.
• The test case files/fixtures can be overruled via the getTestFiles() method
• The property setting can be done inline in the test case/fixture files.

[sirbrillig]

I had tried using AbstractSniffUnitTest back when I first began this fork but got confused and frustrated quickly and found that this method was easier particularly when trying to run the tests on a CI server.

It's very likely that I just didn't understand something but even now a quick search of the wiki doesn't show much beyond the version 3.0 upgrade guide, and that doesn't say very much. Are there docs somewhere about how to write a unit test using AbstractSniffUnitTest?

[jrfnl]

Not really, but I can probably answer most questions you have as I've written plenty ;-)

Having said that, I do understand your frustration and I personally would like an AbstractSniffUnitTest which is slightly more flexible for external standards, so it's on my to do list for PHPCSUtils, though there's a lot on there, so it may be a while before I get to it.

PHPCSUtils does already contain an abstract class to test utility methods for PHPCS with usage documentation in the class docblock: https://github.com/PHPCSStandards/PHPCSUtils/blob/develop/PHPCSUtils/TestUtils/UtilityMethodTestCase.php

[sirbrillig]

It's been a while but one of the issues I believe I had was that the tests seemed to run for a single sniff with little to no explanation of what each message meant. For example, this test is testing this sniff but it only contains one test case which doesn't explain in words what the test does; it could perhaps say testElseIfAsTwoWordsGeneratesWarning which would be much more clear about what that particular test is looking for. Maybe there's a way to do something like that, but I didn't find any examples.

[jrfnl]

With the PHPCS native setup there isn't an easy way to do that, other that to just name the fixtures descriptively and document what you are testing within the fixtures themselves.

I'll keep it in mind for when I start work on the PHPCSUtils sniff test class.