Skip to content

Add PHPUnit test framework and Centralize shell command execution#2498

Open
mattwells wants to merge 2 commits intoFalconChristmas:masterfrom
mattwells:testable-shell-commands
Open

Add PHPUnit test framework and Centralize shell command execution#2498
mattwells wants to merge 2 commits intoFalconChristmas:masterfrom
mattwells:testable-shell-commands

Conversation

@mattwells
Copy link
Contributor

@mattwells mattwells commented Dec 16, 2025

This PR introduces a small, testable abstraction for shell command execution along with a lightweight service container.

Key changes:

  • Added ShellExecutor and ShellResult to centralize and abstract all shell interactions.
  • Introduced a minimal IoC container and global fpp() helper for shared services.
  • Refactored cronjobs.php to use the command executor instead of shell_exec().
  • Added PHPUnit configuration and initial unit tests covering the container, command execution, and cron job behavior.

Why:

  • Makes shell usage explicit, centralized, and mockable.
  • Enables unit testing of code paths that previously required real system calls.
  • Lays groundwork for future refactors without changing runtime behavior.

Notes:

  • No functional changes intended for production behavior.
  • Further refactors (e.g. moving cronjobs to a dedicated class/file) intentionally left for follow-up PRs.

- Replace shell_exec usage with CommandExecutor
- Introduce minimal IoC container and bootstrap
- Add unit tests for command execution, container, and cronjobs
@dkulp
Copy link
Contributor

dkulp commented Dec 16, 2025

My main "issue" with this is terminology. In FPP, "Command" is already somewhat overloaded, but is primarily used for the FPP Command editors and command execution and such. Thus, I'd suggest renaming to something, just not sure what. Maybe something like "ShellExec" or "ShellUtil" or something to denote that it is handling the execution of stuff in a shell.

@mattwells
Copy link
Contributor Author

My main "issue" with this is terminology. In FPP, "Command" is already somewhat overloaded, but is primarily used for the FPP Command editors and command execution and such. Thus, I'd suggest renaming to something, just not sure what. Maybe something like "ShellExec" or "ShellUtil" or something to denote that it is handling the execution of stuff in a shell.

I get that completely, command as a name can very easily become overused (and apparently has). How would fpp()->shell->run('hostname') be?

@mattwells
Copy link
Contributor Author

Done.

@OnlineDynamic
Copy link
Collaborator

My concern with this PR is the introduction of PHPUnit into the core fpp disttribution, doesn't feel like this is required to be installed on every device and just increases the img size and also creates a dev overhead to keep the testing framework up to date and linked to the fpp php version... whilst having some automated tests on fpp would be helpful I think this is something the whole dev team would need to want to progress and maintain - perhaps one for the next dev meeting to discuss... other than a coding exercise what were you trying to actually solve on the change of shell command execution method?

@mattwells
Copy link
Contributor Author

My concern with this PR is the introduction of PHPUnit into the core fpp disttribution, doesn't feel like this is required to be installed on every device and just increases the img size and also creates a dev overhead to keep the testing framework up to date and linked to the fpp php version...

That’s a fair concern. The reason I went with PHPUnit as a PHAR was to keep the amount of change in this PR as small as possible. Ideally PHPUnit would be installed via Composer, but that would mean introducing package management and likely touching the build process as well. Before proposing something that big, I wanted to get a sense of whether testing in the PHP layer is even something the team wants to pursue.

whilst having some automated tests on fpp would be helpful I think this is something the whole dev team would need to want to progress and maintain - perhaps one for the next dev meeting to discuss... other than a coding exercise what were you trying to actually solve on the change of shell command execution method?

I agree this needs to be a team decision. The aim of this PR is to show what testing could look like in practice, rather than just talking about it in theory.

The shell execution abstraction is there because PHP code that calls shell_exec / exec directly is basically impossible to unit test. If the conclusion is that FPP doesn’t want automated tests in the PHP layer, then this PR shouldn’t move forward. If the answer is “yes”, then abstractions like this are a necessary starting point.

The intent here isn’t to change behaviour, just to make existing code testable.

@OnlineDynamic OnlineDynamic changed the title Centralize shell command execution and add unit-testable infrastructure Add PHPUnit test framework and Centralize shell command execution Dec 28, 2025
@agent462
Copy link
Contributor

It would be nice to introduce PHPUnit and Jest for js but fundamental structure changes are needed to implement both successfully. It could be done gradually with no harm though. The structure changes would be good though and moves the project forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants