Skip to content

Skip tests when 'git' command is not available #1105

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

Merged

Conversation

fredden
Copy link
Member

@fredden fredden commented May 26, 2025

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.

Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

jrfnl commented May 26, 2025

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
Copy link
Member Author

fredden commented May 26, 2025

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
Copy link
Member Author

fredden commented May 26, 2025

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
Copy link
Member

jrfnl commented May 26, 2025

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
Copy link
Member

jrfnl commented May 26, 2025

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 fredden force-pushed the tests/skip-when-git-not-available branch from 811718f to d9441d9 Compare May 26, 2025 11:55
@fredden fredden changed the base branch from 4.x to master May 26, 2025 11:55
@fredden
Copy link
Member Author

fredden commented May 26, 2025

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.

Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@jrfnl
Copy link
Member

jrfnl commented May 26, 2025

@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 ?

@fredden fredden force-pushed the tests/skip-when-git-not-available branch 2 times, most recently from 65c5924 to f51ca74 Compare May 26, 2025 14:04
@fredden fredden added this to the 3.13.1 milestone May 26, 2025
This change is intended to be a quick-fix to allow the test suite to pass. A
later enhancement would be to move this check to happen at runtime so that if a
user attempts to use one of these affected filters, a suitable error message
and exit code can be used. When that happens, the tests will need to be
reworked to cover that functionality.
@fredden fredden force-pushed the tests/skip-when-git-not-available branch from f51ca74 to 353bfde Compare May 29, 2025 19:10
@jrfnl jrfnl merged commit 3c4f4be into PHPCSStandards:master May 29, 2025
48 checks passed
@fredden fredden deleted the tests/skip-when-git-not-available branch May 29, 2025 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants