ci: add Dependabot config and Pest unit-test framework - #96
Merged
Merged
Conversation
Cacti core now requires PHP >= 8.2, so the 8.1 integration job fails the composer platform check; Ubuntu Noble also lacks libapache2-mod-php for non-native versions without the ondrej PPA. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved CI configuration and test-bootstrap issues include a critical Apache PHP runtime problem.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds Dependabot automation and Pest unit-test scaffolding while updating CI for supported PHP and Cacti versions.
Changes:
- Adds weekly GitHub Actions Dependabot updates.
- Updates CI for PHP 8.2–8.4, Cacti 1.2.x, SNMP, Pest, and coverage.
- Adds test bootstrap files and Cacti version validation.
File summaries
| File | Summary | Findings |
|---|---|---|
tests/TestCase.php |
Adds the base test case. | No findings. |
tests/Pest.php |
Adds Pest configuration. | Restore the full GPLv2 header (nit, 2 votes). |
tests/bootstrap-unit.php |
Adds Cacti-compatible test bootstrap and stubs. | Add credential-setting stubs; rename the generic source-reading helper; match required Cacti function signatures (moderate, 1–2 votes each). |
tests/.cacti-version |
Defines the expected Cacti version. | No findings. |
.github/workflows/plugin-ci-workflow.yml |
Expands CI and Pest integration. | Register the PPA before PHP setup (moderate, 1 vote); install the Apache PHP module or configure PHP-FPM (critical, 1 vote). |
.github/dependabot.yml |
Configures weekly GitHub Actions updates. | No findings. |
Review details
Suppressed comments (5)
.github/workflows/plugin-ci-workflow.yml:82
- The Ondřej PHP PPA is added only after
setup-phphas already installed PHP and all requested extensions, so this step cannot make the PHP 8.4/snmppackages available during extension setup. Move the PPA registration andapt-get updatebefore theInstall PHPstep; otherwise the new matrix can still fail before the repository is configured.
extensions: intl, mysql, gd, ldap, gmp, xml, curl, json, mbstring, snmp
tests/bootstrap-unit.php:168
- Loading
includes/functions.phpand exercisingservcheck_encrypt_credential()will callread_user_setting()andset_user_setting(), but neither Cacti helper is defined by this bootstrap. The first credential-security test that reaches this path will therefore fail with an undefined-function error instead of testing encryption; add deterministic in-memory stubs for both helpers.
if (!function_exists('read_config_option')) {
function read_config_option($name, $force = false) {
return '';
}
}
tests/bootstrap-unit.php:123
- This stub accepts only two arguments, but the existing plugin calls
db_fetch_row_prepared()with a third$logflag inservcheck_restapi.php:266. Under the PHP 8 CI matrix, exercising that path will fail with an argument-count error before the plugin logic runs; mirror Cacti's optional third parameter.
function db_fetch_row_prepared($sql, $params = array()) {
tests/bootstrap-unit.php:228
- The stub has only one parameter, while existing plugin validation paths pass a filter and an options array (for example,
servcheck_restapi.php:211-235). Tests that invoke those paths will hit an argument-count error instead of the Cacti validation behavior; match the three-argument Cacti signature.
function get_filter_request_var($name) {
tests/bootstrap-unit.php:182
- Cacti's
__()helper accepts formatting/domain arguments, and this plugin already passes three arguments in places such assetup.php:662andservcheck_test.php:475. The two-parameter stub will fail when those paths are exercised; make the test double variadic while retaining the safe default return.
function __($text, $domain = '') {
- Files reviewed: 6/6 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
… avoid cross-plugin collisions
bmfmancini
approved these changes
Sep 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
.github/dependabot.yml) for weekly GitHub Actions updates..github/workflows/plugin-ci-workflow.yml):ppa:ondrej/phpPPA.apache2/libapache2-mod-php, already provided by the PPA.env.CACTI(1.2.x) to match this plugin's compatibility target.snmpPHP extension.pestphp/pest,pestphp/pest-plugin-drift) as dev dependencies.tests/Securityandphpunit.xmlare added in a follow-up PR.env.CACTI != '1.2.x', mirroringCacti/plugin_evidence.tests/directory (TestCase.php,Pest.php,bootstrap-unit.php,.cacti-version), adapted fromCacti/plugin_evidence's test bootstrap for use with servcheck (evidence_test_load()renamed toservcheck_test_load()).Actual Security test cases and
phpunit.xmlwill follow in a separate PR.