refactor(thold): extract the notification delivery from the threshold check - #801
Open
somethingwithproof wants to merge 8 commits into
Open
refactor(thold): extract the notification delivery from the threshold check#801somethingwithproof wants to merge 8 commits into
somethingwithproof wants to merge 8 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR is the next refactor slice for the Thold threshold evaluator, extracting repeated notification-delivery logic out of thold_check_threshold() while adding a PHPUnit-based characterization test harness and CI coverage gate to keep behavior pinned during future restructuring.
Changes:
- Extracted shared mail-delivery logic into
thold_mail_notification()and reused it at several send sites. - Centralized evaluation-time settings into
thold_evaluation_context()to reduce duplicated config/lookup code. - Added a PHPUnit unit-test harness (scenarios/outcomes + characterization tests), patch-level coverage enforcement, and a GitHub Actions workflow to run it in Docker.
Reviewed changes
Copilot reviewed 18 out of 20 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| thold_functions.php | Adds thold_mail_notification() and thold_evaluation_context(), and updates some send sites in thold_check_threshold(). |
| tests/Unit/ThresholdTimeBasedCharacterizationTest.php | Characterization tests for time-based thresholds to pin existing behavior. |
| tests/Unit/ThresholdHiLowCharacterizationTest.php | Characterization tests for hi/low thresholds including the ALERT→WARNING downgrade path. |
| tests/Unit/ThresholdBaselineCharacterizationTest.php | Characterization tests for baseline thresholds using reference statistics fixtures. |
| tests/Unit/TholdMailNotificationTest.php | Direct unit tests for the extracted thold_mail_notification() helper. |
| tests/Unit/TholdEvaluationContextTest.php | Direct unit tests for thold_evaluation_context() behavior (triggers, recipients, attachments). |
| tests/TestCase.php | Base PHPUnit TestCase for resetting stub state and loading plugin sources/constants. |
| tests/Helpers/ThresholdScenario.php | Scenario builder to drive one threshold through one poll of thold_check_threshold(). |
| tests/Helpers/ThresholdOutcome.php | Outcome reader for asserting on side effects (mail/log/traps/persisted columns). |
| tests/Helpers/CactiStubs.php | Recording + programmable stub layer for global Cacti functions used by the plugin. |
| tests/fixtures/optional-core-functions.php | Optional core-function fixtures for exercising function_exists() branches. |
| tests/fixtures/cacti-lib/variables.php | Placeholder fixture file to satisfy runtime include_once() expectations in tests. |
| tests/docker/Dockerfile | Docker image for running unit tests + coverage consistently (PHP 8.1 + pcov). |
| tests/docker/docker-compose.yml | Local docker-compose runner matching CI behavior. |
| tests/bootstrap-unit.php | PHPUnit bootstrap providing global Cacti-function shims and constants for unit tests. |
| tests/bin/patch-coverage.php | Script to enforce coverage on changed PHP lines only (diff-based gate). |
| phpunit.xml | PHPUnit configuration (bootstrap, strictness, and source include). |
| composer.json | Adds dev dependencies (PHPUnit, phplint) and scripts for lint/test/coverage/docker. |
| .gitignore | Ignores vendor, PHPUnit cache, and coverage outputs. |
| .github/workflows/php-unit-tests.yml | GitHub Actions workflow to build the test container, run lint/tests/coverage, and enforce patch coverage. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+2174
to
+2192
| /** | ||
| * Gather the settings a threshold evaluation reads, in one place. | ||
| * | ||
| * Everything here is derived from the threshold row and the Cacti settings; it | ||
| * does not decide anything and has no side effects, which is what lets it move | ||
| * out of thold_check_threshold() without changing behaviour. | ||
| * | ||
| * @param array<string, mixed> $thold_data Threshold row. | ||
| * @param mixed $recipients | ||
| * @param mixed $bcc | ||
| * @param mixed $subject | ||
| * @param mixed $text_type | ||
| * @param mixed $list_id | ||
| * @param mixed $file_array | ||
| * @param mixed $h | ||
| * @param mixed $timespan | ||
| * | ||
| * @return array<string, mixed> | ||
| */ |
Comment on lines
+25
to
+37
| if (!function_exists('db_qstr_rlike')) { | ||
| function db_qstr_rlike($s, $db_conn = false) { | ||
| $s = (string) $s; | ||
|
|
||
| if (strlen($s) > 255) { | ||
| $s = substr($s, 0, 255); | ||
| } | ||
|
|
||
| $s = str_replace(["\0", '|', '{', '}'], '', $s); | ||
|
|
||
| return 'RLIKE ' . db_qstr($s, $db_conn); | ||
| } | ||
| } |
Comment on lines
+39
to
+45
| if (!function_exists('get_total_row_data')) { | ||
| function get_total_row_data($user_id, $sql, $sql_params = [], $class = '', $timeout = 86400) { | ||
| CactiStub::record('get_total_row_data', $sql, $sql_params); | ||
|
|
||
| return CactiStub::nextReturn('get_total_row_data', 0); | ||
| } | ||
| } |
thold_check_threshold() has no return value: everything it decides is a side effect through seven global Cacti functions. ThresholdScenario builds the fixture those need and runs one poll; ThresholdOutcome reads back what was emitted, so a test asserts on behaviour rather than on the SQL text. No production file is touched. Several assertions record behaviour that is wrong rather than intended, each with a comment saying so, so that fixing it later is a deliberate edit here. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Records where this arm has drifted from the hi/low one, notably that a restoral writes the log row and clears the state but sends no mail, so an operator sees the alert and never the all-clear. Cacti's cell fetchers return false rather than '' when a query matches no row. The stub now does the same: on PHP 8 the difference is a TypeError in the re-alert arithmetic, so the old default invented a failure production does not have. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Adopts the conventions from Cacti core: tests/bootstrap-unit.php, tests/Helpers for the stubs, a phpunit.xml carrying error_reporting -1 and CACTI_TEST_BOOTSTRAP, and composer lint / test / test:coverage scripts so CI runs the same commands a developer does. The dev toolchain is Cacti's, pinned to the same platform php 8.1.0. Cacti core runs Pest and this suite does not, because pest ^2 does not resolve on PHP 8.1 -- the platform Cacti's own composer.json pins. Releases up to v2.36.0 conflict with phpunit 10.5.62 and later, every earlier 10.x release is blocked by advisory PKSA-z3gr-8qht-p93v, and v2.36.1, which does resolve, requires PHP 8.2. The stack installs on 8.2 and above; 8.1 is the floor this plugin's CI matrix targets. The tests are written in the plain PHPUnit class style that Cacti's tests/Pest.php explicitly supports, so they run unchanged under Pest wherever it is installable.
This arm compares the reading against statistics rrdtool reports for a reference window, so the scenario supplies those rather than static bounds. Reaching them means answering the three rrdtool calls thold makes -- file existence, an info block describing the data sources and consolidation functions, and a graph command whose printed values are decoded by position -- which the helper now does. Completes the three arms, so Phase 2 can start moving code. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Each was assigned and never read. One of them, $baseu, ran a query against graph_templates_graph and discarded the result, so every threshold on every poll cycle paid for a round trip that fed nothing. $show_datasource likewise called thold_datasource_required() for a value nobody used. The others are $global_alert_address, $deadnotify (issue Cacti#751), $suffix, $show_units, $units_suffix and $decimals. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
The preamble computed the settings the three threshold arms read, so reaching any of that logic meant driving a whole poll. It moves to thold_evaluation_context(), which derives everything from the threshold row and the Cacti settings and decides nothing, and the check unpacks what it needs. Pure motion. The characterization tests from the previous commits pass unchanged, which is what they were written for, and the rules that were buried in the preamble now have direct tests: trigger fallback, whether alerts also reach the warning recipients, and when a graph is attached. thold_check_threshold() goes from 1432 lines to 1378. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
… check Every send site resolved the notification list's format file, then mailed the list unless it was empty or the threshold had been acknowledged. That pair is now thold_mail_notification(), and the five sites whose shape matched exactly call it. The body is still built inside the guard rather than by the caller, because composing it resolves the device's site and an empty recipient list should not pay for a message nobody receives. The function returns the message so the sites that mail a second list can reuse it as before. One observable difference: where the primary list is empty, $message is now '' rather than unset. The second mail already read that unset variable, so this removes an undefined-variable read without changing what the mail contains. thold_check_threshold() goes from 1378 lines to 1343. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
A threshold already in alert whose reading falls back into the warning band notifies a downgrade rather than a restoral. Reaching it needs both fail counts at or above their triggers on the same poll, which none of the earlier scenarios set up. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
somethingwithproof
force-pushed
the
refactor/notification-emitter
branch
from
August 17, 2026 04:42
e51085d to
a70db77
Compare
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.
Phase 3, first slice.
Stacked on #800, which is stacked on #799. Merge order is #799, #800, then this. Until they land this PR's diff carries theirs.
What moved
Every send site in
thold_check_threshold()did the same two things: resolve the notification list's format file, then mail the list unless it was empty or the threshold had been acknowledged. There are 18 of those pairs.That is now
thold_mail_notification(). Five sites matched the shape exactly and call it; the other thirteen differ in ways that are themselves the subject of open issues, so they stay put until Phase 4 decides which form is correct.Two details worth flagging, because both were judgement calls:
get_thold_alert_text(), which resolves the device's site through a query, so hoisting it would make an empty recipient list pay for a message nobody receives. The function takes the text class and builds lazily instead.The one observable difference
Where the primary list is empty,
$messageis now''rather than unset. The second mail already read that unset variable, so this removes an undefined-variable read. It cannot change what a mail contains —nulland''render identically in the body — but it is a difference and it belongs in the description rather than buried.thold_check_threshold()goes from 1378 lines to 1343.A correction to my own earlier analysis
Coverage flagged one converted line as unreached: the
ALERT > WARNINGbranch in the hi/low arm. I had previously described that branch as unreachable, and #752 describes its$suspend_notifyguard as structurally dead.Rather than assume, I tried to reach it. It is reachable. With a threshold already in alert, a reading falling back into the warning band, and both fail counts at or above their triggers, it fires and logs
ST_NOTIFYAWwith anALERT > WARNINGsubject. My earlier claim was wrong; the line was uncovered because none of the scenarios set both counters up that way.There is now a characterization test for it. #752's narrower claim about the
$suspend_notifyguard is untouched by this and still stands on its own.Verification
61 tests, 100% of changed lines covered. The characterization tests pass unchanged apart from the one added above.
composer lintandcomposer testclean.Next
The remaining thirteen sites differ on: whether a second list is mailed, whether a trigger command runs, which SNMP payload is built, and whether
graph_timespanis passed. Each difference maps to a filed issue, so the rest of Phase 3 lands alongside Phase 4 rather than before it.