test: add Pest test harness with Security/Unit/Integration coverage - #105
Merged
Merged
Conversation
This plugin had no Pest test infrastructure at all (only a placeholder tests/bootstrap-unit.php and tests/.cacti-version) - the existing CI workflow already gracefully skips its Pest step when phpunit.xml or tests/Security are absent. Added the standard harness used across the rest of the plugin fleet and populated it with real coverage: - Security: PHP compatibility scan (8.0-8.4 removed/added syntax) and a static structure check (required functions, INFO keys). - Unit: plugin_servcheck_version(), plugin_servcheck_uninstall() (drops all 9 tables), plugin_servcheck_setup_table() (creates all 6 tables via the plugin table-creation API), plugin_servcheck_upgrade() (the always-run steps once already past both version checkpoints: realm-file update, hook re-registration, final plugin_config UPDATE), plugin_servcheck_config_arrays() (menu entry, version-check page gating), plugin_servcheck_draw_navigation_text(), servcheck_config_settings(), servcheck_page_head(), servcheck_replicate_out()'s non-"all" no-op branch, plugin_servcheck_poller_bottom(). - Integration: plugin_servcheck_install() (verifies every hook and the realm it registers, together with its full table set, end-to-end). Intentionally NOT covered: plugin_servcheck_upgrade()'s two large version-gated migration cascades (0.3 and 0.4). These perform irreversible schema changes (table renames/drops, backup-table creation) and convert real credential data through servcheck_encrypt_credential() (real encryption via includes/functions.php, not a pure function safe to assert against in a unit test) - a much larger and riskier surface than this suite stubs. Bootstrap additions (tests/bootstrap-unit.php, previously nearly empty): api_plugin_register_hook/api_plugin_register_realm (logging), get_current_page/test_set_current_page, cacti_version_compare, db_table_exists, db_add_column/db_remove_column (logging), db_fetch_insert_id, get_selected_theme, exec_background (logging - safe here, plugin_servcheck_poller_bottom() include_once()s $config['library_path'] not the real lib/poller.php), replicate_out_table, and a settable db_fetch_cell fixture. Also fixed a stale tests/.cacti-version pin (1.2.31 -> 1.2.x) that made every local/CI run fail its own bootstrap version-match guard against a newer Cacti checkout. Verified via a real WSL Cacti install: 25 passed (255 assertions).
TheWitness
requested review from
bmfmancini and
xmacan
and
a lite review from Copilot
September 22, 2026 11:49
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Moderate CI-selection and test-coverage issues remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 5
Open (5)
What changed in this PR
Adds a Pest testing harness with Security, Unit, and Integration coverage for the servcheck plugin.
Changes:
- Adds PHPUnit/Pest configuration and Cacti-compatible test bootstrap stubs.
- Adds lifecycle, setup, upgrade, navigation, poller, compatibility, and installation tests.
- Updates the Cacti version fixture to support the moving
1.2.xbranch.
| File | Description |
|---|---|
tests/Unit/ServcheckUpgradeTest.php |
Tests upgrade behavior after migration checkpoints. |
tests/Unit/ServcheckSetupTableTest.php |
Tests table provisioning. |
tests/Unit/ServcheckPollerBottomTest.php |
Tests poller dispatch behavior. |
tests/Unit/ServcheckLifecycleTest.php |
Tests version and uninstall behavior. |
tests/Unit/ServcheckConfigAndNavigationTest.php |
Tests configuration and navigation helpers. |
tests/Security/SetupStructureTest.php |
Validates setup structure and metadata. |
tests/Security/PhpCompatibilityTest.php |
Checks PHP compatibility syntax. |
tests/Integration/ServcheckInstallHooksTest.php |
Tests installation hooks, realm, and tables. |
tests/bootstrap-unit.php |
Adds Cacti test doubles and fixtures. |
tests/.cacti-version |
Allows testing against the moving Cacti 1.2.x branch. |
phpunit.xml |
Defines Pest test suites and coverage configuration. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- CI workflow's Pest invocation only ran tests/Security, silently skipping tests/Unit and tests/Integration entirely (phpunit.xml already lists all three testsuite directories). Drop the explicit path argument so the full suite runs. - Strengthen assertions across ServcheckInstallHooksTest, ServcheckLifecycleTest, and ServcheckSetupTableTest to check exact hook function names and the full/exact set of table names instead of a loose subset/count check. - ServcheckUpgradeTest: broaden the "no migration cascade" assertion to also check for absent CREATE TABLE/DROP TABLE calls, not just RENAME COLUMN.
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
This plugin had no Pest test infrastructure at all (only a placeholder
tests/bootstrap-unit.phpandtests/.cacti-version) - the existing CI workflow already gracefully skips its Pest step whenphpunit.xmlortests/Securityare absent. This PR builds the standard harness used across the rest of the plugin fleet and populates it with real coverage.Test coverage added
plugin_servcheck_version(),plugin_servcheck_uninstall()(drops all 9 tables),plugin_servcheck_setup_table()(creates all 6 tables via the plugin table-creation API),plugin_servcheck_upgrade()(the always-run steps once already past both version checkpoints: realm-file update, hook re-registration, finalplugin_configUPDATE),plugin_servcheck_config_arrays()(menu entry, version-check page gating),plugin_servcheck_draw_navigation_text(),servcheck_config_settings(),servcheck_page_head(),servcheck_replicate_out()'s non-"all"no-op branch,plugin_servcheck_poller_bottom().plugin_servcheck_install()- verifies every hook and the realm it registers, together with its full table set, in one end-to-end pass.Intentionally not covered
plugin_servcheck_upgrade()'s two large version-gated migration cascades (0.3 and 0.4). These perform irreversible schema changes (table renames/drops, backup-table creation) and convert real credential data throughservcheck_encrypt_credential()(real encryption viaincludes/functions.php, not a pure function safe to assert against in a unit test) - a much larger and riskier surface than this suite stubs.Bootstrap additions (
tests/bootstrap-unit.php, previously nearly empty)api_plugin_register_hook/api_plugin_register_realm(logging),get_current_page/test_set_current_page,cacti_version_compare,db_table_exists,db_add_column/db_remove_column(logging),db_fetch_insert_id,get_selected_theme,exec_background(logging - safe here,plugin_servcheck_poller_bottom()include_once()s$config['library_path'], not the reallib/poller.php),replicate_out_table, and a settabledb_fetch_cellfixture.Other fix
tests/.cacti-versionwas pinned to a stale exact release (1.2.31), which made every local/CI run fail its own bootstrap version-match guard against a newer Cacti checkout. Corrected to1.2.x.Result
Verified via a real WSL Cacti install: