-
-
Notifications
You must be signed in to change notification settings - Fork 77
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
Skip tests when 'git' command is not available #1105
Conversation
There was a problem hiding this 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:
- First, I'd like to understand the situation where the tests would be run without
git
being available.
While this if definitely possible in the3.x
branch, as of4.x
, the tests are no longer shipped when the package is downloaded (via Composer or the GH zips), so this should not be possible. - Which brings me to the next question: why was this pulled against
4.x
? The tests also exist in the3.x
branch. - 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 thegit
command ?
Now, it would be a little silly, of course, for someone to run theGitModified
filter if they are not in agit
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)....
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 |
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. |
I have been using
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. |
Understood. That makes sense. |
🙌🏻
Yes please. AFAICS, the affected tests also exist in
In that case, this does more than is needed. See my remark here.
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. |
811718f
to
d9441d9
Compare
I have created #1107 to track this enhancement suggestion. |
There was a problem hiding this 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.
@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 |
65c5924
to
f51ca74
Compare
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.
f51ca74
to
353bfde
Compare
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 thegit
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
PR checklist