Skip tests when 'git' command is not available

[fredden]

Description

While running the test suite for this repository within a limited environment, several tests were failing without the git command-line tool available. This change skips these tests when the git command is not found.

To reproduce the "no git available" scenario, check out a copy of this repository, and run: docker run --rm -it -w /work -v "$PWD:/work" php:8.4-cli-alpine vendor/bin/phpunit --no-coverage --testdox --filter=Git

Suggested changelog entry

None required

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.

[jrfnl]

Thanks for this PR @fredden.

Looking at the changes, I have a number of questions:

1. First, I'd like to understand the situation where the tests would be run without git being available.
   While this if definitely possible in the 3.x branch, as of 4.x, the tests are no longer shipped when the package is downloaded (via Composer or the GH zips), so this should not be possible.
2. Which brings me to the next question: why was this pulled against 4.x ? The tests also exist in the 3.x branch.
3. And then the bigger/more structural question: why is this a test-only change ?
   What I mean by this is: why is the check for git being available being done in the tests, while the runtime code actually executes the git command ?
   Now, it would be a little silly, of course, for someone to run the GitModified filter if they are not in a git environment, but it could be a beginner error and maybe, we should be showing a friendly error message in that case.
   What do you think ?
   Of course, if you'd go that route, different test changes would be needed (and the new error message would need to be covered by tests too)....

[jrfnl]

Looking even closer at these tests, I understand even less of the test failure you mention.

The majority of the tests mock the output for git and only the testExecAlwaysReturnsArray() test method executes the git command - and that test method has a check that the .git directory is available and skips otherwise.
Now, it could be argued that checking for the .git directory being available is not enough and that that tests should check for the git command being available instead (or in addition to), and I'd be open to a change to that test method (in both test classes), but I don't see why this PR makes changes to the other test methods which don't actually run the git command.
And if that would really be needed, would it maybe make sense to move the check (and the test skip) to a setUp() method ?

[fredden]

First, I'd like to understand the situation where the tests would be run without git being available.

I have a certain version of PHP installed on my computer. (Currently v8.4, but that doesn't matter here.) In order to run the test-suite on a different PHP version, I have used Docker. I've added an example reproduction line to the pull request description. The PHP version number can be changed as needed.

[fredden]

why was this pulled against 4.x ? The tests also exist in the 3.x branch.

I have been using 4.x as a base recently. I can rebase this onto 3.x (or rather "master" for now) if you would prefer. I have not tested if this bug/limitation exists in other branches.

why is this a test-only change?

Because I wanted to unblock the actual task that I'm working on. Yes, detecting a required dependency at runtime for these filters sounds like a sensible enhancement that can be added. If/when that gets added, the tests need to be adjusted to cover the new code; at that point I would expect this change to be replaced with something that covers the new code/behaviour.

[jrfnl]

I have a certain version of PHP installed on my computer. (Currently v8.4, but that doesn't matter here.) In order to run the test-suite on a different PHP version, I have used Docker. I've added an example reproduction line to the pull request description. The PHP version number can be changed as needed.

Understood. That makes sense.
Still leaves the other remarks/questions to be addressed.

[jrfnl]

I have been using 4.x as a base recently.

🙌🏻

I can rebase this onto 3.x (or rather "master" for now) if you would prefer. I have not tested if this bug/limitation exists in other branches.

Yes please. AFAICS, the affected tests also exist in master, so this should be addressed in master (and then merged up to 4.x).

Because I wanted to unblock the actual task that I'm working on.

In that case, this does more than is needed. See my remark here.

Yes, detecting a required dependency at runtime for these filters sounds like a sensible enhancement that can be added. If/when that gets added, the tests need to be adjusted to cover the new code; at that point I would expect this change to be replaced with something that covers the new code/behaviour.

Exactly. I have no objections to merging the "quick fix" for now (once rebased and then updated to only skip for those tests affected), but I think the enhancement would be the preferred direction. If you prefer for this PR to just be the quick fix, then please open an issue to remind us to address the enhancement at a later point in time.

[fredden]

please open an issue to remind us to address the enhancement at a later point in time.

I have created #1107 to track this enhancement suggestion.

[jrfnl]

Thanks for making those updates @fredden ! I'll merge this once I'm back in the office.

[jrfnl]

@fredden Just noticed CI isn't "finishing" as you pushed before changing the base branch. Could you (force-)push again without changes, to get the CI to run the correct checks for master ?