test: add Security baseline and expand Unit/Integration coverage for lifecycle - #246
Merged
Merged
Conversation
…lifecycle This plugin had no tests/Security category at all, and setup.php's lifecycle/device-hook functions had zero test coverage (only db_functions.php and the request-output escaping/wiring were tested). Closed both gaps: - Security: added the standard PHP compatibility scan (8.0-8.4 removed/added syntax) and a static structure check (required functions, INFO keys), matching the baseline used across the rest of the plugin fleet. - Unit: plugin_monitor_version(), the lifecycle contract functions (plugin_monitor_uninstall - verifies it drops every table it owns, plugin_monitor_check_config - both the out-of-range and valid monitor_refresh branches, plugin_monitor_upgrade, monitor_check_upgrade()'s page-guard and version-drift branches), the device-list hooks (monitor_device_filters, monitor_device_sql_where, monitor_device_action_array, monitor_device_remove), monitor_draw_navigation_text(), monitor_get_default(), monitor_setup_table() (verifies every table it provisions), and monitor_poller_bottom() (primary-poller and non-primary-poller branches). - Integration: plugin_monitor_install() (verifies every hook, the realm, the tables it provisions, and the config defaults it sets, together, in one pass) - this plugin had no tests/Integration coverage of setup.php before (the existing Integration tests cover request-output wiring, not the plugin lifecycle). No new unserialize() hardening was needed: setup.php's one unserialize() call already goes through sanitize_unserialize_selected_items(). Also fixed tests/.cacti-version, which was pinned to a stale "1.2.31" and failed the bootstrap's version check against any other real Cacti checkout (e.g. a local 1.2.32 checkout); "1.2.x" matches the moving-branch convention used elsewhere in this fleet. Intentionally left uncovered: monitor_device_table_bottom()/monitor_show_tab()/ monitor_top_graph_refresh()/plugin_monitor_page_head() (print-heavy UI rendering), monitor_config_settings()/monitor_config_arrays()/monitor_config_form() (very large), monitor_device_action_execute()/monitor_device_action_prepare() (large, print/DB-heavy confirmation dialogs), and monitor_api_device_save() (moderate complexity, comparatively low value relative to the effort of safely mocking its dependencies) - consistent with similarly-scoped functions skipped elsewhere in this coverage-expansion pass. tests/bootstrap-unit.php gained a fixture-mock DB API (monitor_test_mock_db/monitor_test_reset_db_mocks/monitor_test_db_result, matching the convention used elsewhere in the plugin fleet), api_plugin_register_hook/api_plugin_register_realm call-logging stubs, db_table_exists, and exec_background (call-logging, safe here since setup.php's poller include always goes through $config['library_path'], which points at a throwaway test stub, never Cacti core's real lib/poller.php); read_config_option/set_config_option were rewired from hardcoded no-ops to a real backing store - all needed by the new tests. Verified via a real WSL Cacti install (composer-managed Pest, not a plugin-local vendor tree): 63 passed (303 assertions), up from 31.
TheWitness
requested review from
bmfmancini and
xmacan
and
a lite review from Copilot
September 22, 2026 11:11
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Several lifecycle tests do not assert the behavior they claim to cover, and the uninstall test misses the dashboard table provisioned by the plugin.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 4
Open (4)
What changed in this PR
Expands lifecycle, device-hook, security, and compatibility coverage for the Monitor plugin and updates test bootstrap/version handling.
Changes:
- Adds unit and integration tests for setup lifecycle, hooks, tables, defaults, and poller behavior.
- Adds PHP compatibility and setup-structure security tests.
- Extends test mocks and registers the Security suite.
| File | Description |
|---|---|
tests/Unit/MonitorVersionTest.php |
Tests plugin metadata parsing. |
tests/Unit/MonitorSetupTableAndPollerTest.php |
Tests table setup and poller launching. |
tests/Unit/MonitorLifecycleTest.php |
Tests lifecycle and upgrade behavior. |
tests/Unit/MonitorDeviceHooksTest.php |
Tests device hooks and defaults. |
tests/Security/SetupStructureTest.php |
Validates setup structure and INFO keys. |
tests/Security/PhpCompatibilityTest.php |
Scans for incompatible PHP syntax. |
tests/Integration/MonitorInstallHooksTest.php |
Tests installation registrations and defaults. |
tests/bootstrap-unit.php |
Adds database, configuration, hook, and execution mocks. |
tests/.cacti-version |
Uses the moving Cacti 1.2.x baseline. |
phpunit.xml |
Includes Security tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
bmfmancini
approved these changes
Sep 23, 2026
bmfmancini
left a comment
Member
There was a problem hiding this comment.
Approved — verified this is the only open PR in the Cacti org with this title. Nice coverage expansion for the lifecycle and device-hook paths.
xmacan
approved these changes
Sep 23, 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.

This plugin had no
tests/Securitycategory at all, andsetup.php's lifecycle/device-hook functions had zero test coverage (onlydb_functions.phpand the request-output escaping/wiring were tested). Closed both gaps, running the full suite against a real Cacti install in WSL (composer-managed Pest, not a plugin-local vendor tree) until everything passed.Added coverage
plugin_monitor_version(), the lifecycle contract functions (plugin_monitor_uninstall— verifies it drops every table it owns,plugin_monitor_check_config— both the out-of-range and validmonitor_refreshbranches,plugin_monitor_upgrade,monitor_check_upgrade()'s page-guard and version-drift branches), the device-list hooks (monitor_device_filters,monitor_device_sql_where,monitor_device_action_array,monitor_device_remove),monitor_draw_navigation_text(),monitor_get_default(),monitor_setup_table()(verifies every table it provisions), andmonitor_poller_bottom()(primary-poller and non-primary-poller branches).plugin_monitor_install()(verifies every hook, the realm, the tables it provisions, and the config defaults it sets, together, in one pass) — this plugin had notests/Integrationcoverage of setup.php before (the existing Integration tests cover request-output wiring, not the plugin lifecycle).Security fix
No new
unserialize()hardening was needed:setup.php's oneunserialize()call already goes throughsanitize_unserialize_selected_items().Also fixed
tests/.cacti-versionwas pinned to a stale1.2.31and failed the bootstrap's version check against any other real Cacti checkout (e.g. a local 1.2.32 checkout);1.2.xmatches the moving-branch convention used elsewhere in this fleet.Intentionally not covered
monitor_device_table_bottom()/monitor_show_tab()/monitor_top_graph_refresh()/plugin_monitor_page_head()(print-heavy UI rendering),monitor_config_settings()/monitor_config_arrays()/monitor_config_form()(very large),monitor_device_action_execute()/monitor_device_action_prepare()(large, print/DB-heavy confirmation dialogs), andmonitor_api_device_save()(moderate complexity, comparatively low value relative to the effort of safely mocking its dependencies) — consistent with similarly-scoped functions skipped elsewhere in this coverage-expansion pass.Test infrastructure
tests/bootstrap-unit.phpgained a fixture-mock DB API (monitor_test_mock_db/monitor_test_reset_db_mocks/monitor_test_db_result, matching the convention used elsewhere in the plugin fleet),api_plugin_register_hook/api_plugin_register_realmcall-logging stubs,db_table_exists, andexec_background(call-logging, safe here since setup.php's poller include always goes through$config['library_path'], which points at a throwaway test stub, never Cacti core's reallib/poller.php);read_config_option/set_config_optionwere rewired from hardcoded no-ops to a real backing store.Result
Tests: 63 passed (303 assertions), up from 31.