Move test directory out of package path

[sirbrillig]

This moves the tests from VariableAnalysis/Tests to Tests.

Related to #116 and #162

[sirbrillig]

cc @jrfnl

What do you think about this organization?

[jrfnl]

Definitely a good step in the right direction. 👍

Next step, IMO, would be to split the VariableAnalysisTest.php into separate files, linked one-on-one with the fixture files.

Couple of small remarks/questions for this PR:

• Should the fixtures directory be under the tests/Sniffs directory ? Or should the VariableAnalysisTest.php file possibly not be in a Sniffs subdirectory ?
  Having them as "equal" subdirectories seem to create a disconnect in their relation to each other.
• As you're changing the phpcs.xml.dist ruleset, should vendor be excluded as well ?

[sirbrillig]

Next step, IMO, would be to split the VariableAnalysisTest.php into separate files

Agreed.

Should the fixtures directory be under the tests/Sniffs directory? Or should the VariableAnalysisTest.php file possibly not be in a Sniffs subdirectory?

Yes, good point. I'm not sure why I had moved it up a level. As for the Sniffs subdirectory, I don't particularly care one way or another; do you know if there's a common pattern for this? I imagine I had it this way for a reason but that reason is lost to time 😅

As you're changing the phpcs.xml.dist ruleset, should vendor be excluded as well ?

👍

One last thing to consider is the test namespace fixed in #164. This leaves that change in place, but I'm not sure if it's still correct.

[jrfnl]

One last thing to consider is the test namespace fixed in #164. This leaves that change in place, but I'm not sure if it's still correct.

Good point, that will need fixing now. Would you like me to guide you through possible solutions or shall I add a commit to this branch with a fix ?

[sirbrillig]

You're welcome to commit to this branch!

[jrfnl]

You're welcome to commit to this branch!

Done, the additional commit should fix it. See the commit message for the rationale.

There's just one thing left I'm still a bit unsure about: the naming of the tests/Sniffs directory.
That kind of gives the impression that there are more sniffs in this standard and that each sniff will have a test file there, while the reality is that there is one sniff and AFAIK no intention of additional sniffs.

Especially with an eye of the test file being split in the future, would it make sense to call the directory tests/VariableAnalysisSniff ?

That would also allow for additional sniffs - if the sniff would be split or other variable analysis sniffs would be added - to each have their own tests/ subdirectory with the tests which are specific to that sniff.

[jrfnl]

Oh... last thing which just popped up in my mind: should the tests directory name be capitalized ?

[sirbrillig]

Done, the additional commit should fix it. See the commit message for the rationale.

Perfect. Thank you! 👌

There's just one thing left I'm still a bit unsure about: the naming of the tests/Sniffs directory.
That kind of gives the impression that there are more sniffs in this standard and that each sniff will have a test file there, while the reality is that there is one sniff and AFAIK no intention of additional sniffs. Especially with an eye of the test file being split in the future, would it make sense to call the directory tests/VariableAnalysisSniff ?

Yeah, the directories were just set up to mirror the ones in the source directories, but I don't know why I chose to do that (presumably I was copying an existing structure somewhere). I think tests/VariableAnalysisSniff sounds fine. To play devil's advocate, is there a reason not to put all the test files at the top level of tests/ rather than having any subdirectories at all?

should the tests directory name be capitalized?

That's a good question. My habit from other packages and languages is to leave everything lowercase unless required, hence tests, but if there's a composer standard to capitalize the Tests directory that's fine with me. Do you usually capitalize it?

[jrfnl]

Yeah, the directories were just set up to mirror the ones in the source directories, but I don't know why I chose to do that (presumably I was copying an existing structure somewhere).

Well, if it would mirror the src, I'd expect tests/Sniffs/CodeAnalysis/ or tests/Sniffs/CodeAnalysis/VariableAnalysisSniff.

I think tests/VariableAnalysisSniff sounds fine. To play devil's advocate, is there a reason not to put all the test files at the top level of tests/ rather than having any subdirectories at all?

The only reason I can think of, is that having the actual tests in a subdirectory vs the test utility files (base test case + bootstrap) in the tests root, provides a clear separation between the two types of files in the tests directory. Similar to the fixtures files also being separated from the tests.

Is that enough of a reason ?

My habit from other packages and languages is to leave everything lowercase unless required, hence tests, but if there's a composer standard to capitalize the Tests directory that's fine with me. Do you usually capitalize it?

I'm thinking more of PSR-4 and the different OSes.

The base namespace for the tests is VariableAnalysis\Tests and the root directory is tests.
This is, in a way, a mismatch of case - Tests vs tests -. It should be ok as we've provided tests as an entry path for the VariableAnalysis\Tests namespace now in composer.json and for people on Windows it won't be a problem anyway as the file system is case-insensitive, but for Linux/MacOS, there could potentially be an issue with case-sensitivity.

Also - though a PHPCS requirement - the VariableAnalysis directory and its subdirectories are also first letter capitalized, so it just feels a bit odd compared to that ;-)

[sirbrillig]

The only reason I can think of, is that having the actual tests in a subdirectory vs the test utility files (base test case + bootstrap) in the tests root, provides a clear separation between the two types of files in the tests directory. Similar to the fixtures files also being separated from the tests.

Sounds good to me! I'll move it shortly.

This is, in a way, a mismatch of case - Tests vs tests -. It should be ok as we've provided tests as an entry path for the VariableAnalysis\Tests namespace now in composer.json and for people on Windows it won't be a problem anyway as the file system is case-insensitive, but for Linux/MacOS, there could potentially be an issue with case-sensitivity.

Good points, all. Thanks for leading me through the logic there. I'll rename it!

[jrfnl]

LGTM 👍