From 46b8a531948557c5087637c6c3fc4509dbcf608f Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sun, 16 Aug 2026 23:37:33 -0700 Subject: [PATCH 1/4] fix(thold): make the unit suffix conversions inverses of each other thold_display_to_raw() and thold_raw_to_display() each carried their own suffix table and disagreed. Entering 'p' scaled by 1e-9, while a value of 1e-12 rendered as 'f' and so parsed back as 1e-15: opening a threshold and saving it again divided its bound by a thousand, every time. Nano was missing from both, which is what pushed the rest of the small suffixes one place out of step. Past the largest and smallest entries the render read off the end of its pattern string and dropped the magnitude entirely, so 5e-18 displayed as 5. Both now read one shared table, so they cannot drift apart again, and a value outside the table keeps its scale rather than losing it. Stored bounds are untouched. What changes is the label they display under and the meaning of a newly typed suffix: 'p' is pico, as it always claimed to be. Refs #786 Signed-off-by: Thomas Vincent --- tests/Unit/TholdUnitSuffixTest.php | 173 +++++++++++++++++++++++++++++ tests/docker/Dockerfile | 29 +++++ tests/docker/docker-compose.yml | 15 +++ thold_functions.php | 163 +++++++++------------------ 4 files changed, 269 insertions(+), 111 deletions(-) create mode 100644 tests/Unit/TholdUnitSuffixTest.php create mode 100644 tests/docker/Dockerfile create mode 100644 tests/docker/docker-compose.yml diff --git a/tests/Unit/TholdUnitSuffixTest.php b/tests/Unit/TholdUnitSuffixTest.php new file mode 100644 index 00000000..9627a527 --- /dev/null +++ b/tests/Unit/TholdUnitSuffixTest.php @@ -0,0 +1,173 @@ + + */ + public static function suffixProvider() { + return [ + 'femto' => ['5f', 5.0e-15], + 'pico' => ['5p', 5.0e-12], + 'nano' => ['5n', 5.0e-9], + 'micro' => ['5u', 5.0e-6], + 'milli' => ['5m', 5.0e-3], + 'kilo' => ['5K', 5.0e3], + 'mega' => ['5M', 5.0e6], + 'giga' => ['5G', 5.0e9], + 'tera' => ['5T', 5.0e12], + 'peta' => ['5P', 5.0e15], + 'exa' => ['5E', 5.0e18], + 'zetta' => ['5Z', 5.0e21], + 'yotta' => ['5Y', 5.0e24], + ]; + } + + /** + * @dataProvider suffixProvider + * + * @param string $typed + * @param float $stored + * + * @return void + */ + public function testEachSuffixScalesByItsSiFactor($typed, $stored): void { + $this->assertEqualsWithDelta($stored, thold_display_to_raw($typed, 'thold_hi'), abs($stored) * 1.0e-9); + } + + /** + * @dataProvider suffixProvider + * + * @param string $typed + * @param float $stored + * + * @return void + */ + public function testEachStoredValueRendersWithItsSiSuffix($typed, $stored): void { + $this->assertSame($typed, thold_raw_to_display($stored)); + } + + /** + * Opening a threshold and saving it again must not change it. This is the + * failure that mattered: a value stored at 1e-12 rendered as 5f, which + * parsed back as 1e-15, so every visit to the form divided it by a + * thousand. + * + * @dataProvider suffixProvider + * + * @param string $typed + * @param float $stored + * + * @return void + */ + public function testAValueSurvivesBeingDisplayedAndReEntered($typed, $stored): void { + $round_tripped = thold_display_to_raw(thold_raw_to_display($stored), 'thold_hi'); + + $this->assertEqualsWithDelta($stored, $round_tripped, abs($stored) * 1.0e-9); + } + + /** + * @return void + */ + public function testAPlainNumberIsLeftAlone(): void { + $this->assertSame('42', thold_display_to_raw('42', 'thold_hi')); + $this->assertSame('42', thold_raw_to_display(42)); + } + + /** + * @return void + */ + public function testZeroIsLeftAlone(): void { + $this->assertSame('0', thold_raw_to_display(0)); + } + + /** + * @return void + */ + public function testNegativeValuesKeepTheirSign(): void { + $this->assertSame('-5K', thold_raw_to_display(-5000)); + $this->assertEqualsWithDelta(-5000, thold_display_to_raw('-5K', 'thold_hi'), 1.0e-6); + } + + /** + * @return array + */ + public static function rejectedInputProvider() { + return [ + 'unknown suffix' => ['5x'], + 'letters only' => ['abc'], + 'empty' => [''], + ]; + } + + /** + * @dataProvider rejectedInputProvider + * + * @param string $typed + * + * @return void + */ + public function testUnusableInputIsRejectedAndFlagged($typed): void { + $this->assertFalse(thold_display_to_raw($typed, 'thold_hi')); + $this->assertArrayHasKey('thold_hi', $_SESSION['sess_error_fields']); + } + + /** + * @return void + */ + public function testNonNumericInputHasNoDisplayForm(): void { + $this->assertFalse(thold_raw_to_display('abc')); + } + + /** + * Beyond the largest and smallest suffix there is nothing left to index, + * and the old code read past the end of the pattern and dropped the + * magnitude entirely. + * + * @return void + */ + public function testMagnitudesBeyondTheLargestSuffixKeepTheirScale(): void { + $rendered = thold_raw_to_display(5.0e27); + + $this->assertNotSame('5', $rendered); + $this->assertEqualsWithDelta(5.0e27, (float) thold_display_to_raw($rendered, 'thold_hi'), 5.0e18); + } + + /** + * @return void + */ + public function testMagnitudesBelowTheSmallestSuffixKeepTheirScale(): void { + $rendered = thold_raw_to_display(5.0e-18); + + $this->assertNotSame('5', $rendered); + $this->assertEqualsWithDelta(5.0e-18, (float) thold_display_to_raw($rendered, 'thold_hi'), 5.0e-27); + } +} diff --git a/tests/docker/Dockerfile b/tests/docker/Dockerfile new file mode 100644 index 00000000..518f7321 --- /dev/null +++ b/tests/docker/Dockerfile @@ -0,0 +1,29 @@ +# Test runner for the Thold plugin. +# +# Pinned to PHP 8.1 because that is the oldest interpreter the CI matrix +# covers; what passes here passes on 8.2-8.4. pcov rather than Xdebug: line +# coverage is the only debug feature the suite needs and pcov is far cheaper. +FROM php:8.1-cli-alpine@sha256:7949370448b0b4d9787776dc5968e0fd8d48763292344b5fbf21539441228a98 + +# git is needed by the changed-line coverage gate, which diffs against the +# base branch. +RUN apk add --no-cache git gmp-dev \ + && docker-php-ext-install gmp \ + && apk add --no-cache --virtual .build-deps $PHPIZE_DEPS \ + && pecl install pcov \ + && docker-php-ext-enable pcov \ + && apk del .build-deps + +COPY --from=composer:2@sha256:4d71c3c2109c61d5415544264b59ad4087e4c5b7244481723664138fd36d5040 /usr/bin/composer /usr/bin/composer + +# The plugin lives where Cacti would put it, because thold_functions.php +# resolves its own includes through $config['base_path'] . '/plugins/thold'. +# No network or database is involved; the Cacti framework functions themselves +# are stubbed in tests/bootstrap.php. +WORKDIR /cacti/plugins/thold + +ENV COMPOSER_ALLOW_SUPERUSER=1 \ + COMPOSER_NO_INTERACTION=1 \ + COMPOSER_CACHE_DIR=/tmp/composer-cache + +CMD ["sh", "-c", "composer install --no-progress --no-ansi && composer test"] diff --git a/tests/docker/docker-compose.yml b/tests/docker/docker-compose.yml new file mode 100644 index 00000000..99d38b47 --- /dev/null +++ b/tests/docker/docker-compose.yml @@ -0,0 +1,15 @@ +# Local mirror of the unit-test CI job. `docker compose -f +# tests/docker/docker-compose.yml run --rm phpunit` runs exactly what CI runs. +services: + phpunit: + build: + context: . + dockerfile: Dockerfile + image: cacti-thold-test:php8.1 + working_dir: /cacti/plugins/thold + volumes: + - ../..:/cacti/plugins/thold + - composer-cache:/tmp/composer-cache + +volumes: + composer-cache: diff --git a/thold_functions.php b/thold_functions.php index 018cf99b..6d25c94c 100644 --- a/thold_functions.php +++ b/thold_functions.php @@ -5279,6 +5279,40 @@ function thold_create_new_graph_from_template() { * @param mixed $number * @param mixed $field_name */ +/** + * SI suffixes thold accepts on a threshold bound, smallest first. + * + * thold_display_to_raw() and thold_raw_to_display() are inverses of each + * other, so they read the same table rather than each carrying their own + * copy. They previously disagreed: 'p' scaled by 1e-9 on the way in while + * 1e-12 rendered as 'f' on the way out, so a bound was divided by a thousand + * every time its form was opened and saved. + * + * @return array Suffix to the factor it multiplies by. + */ +function thold_unit_suffixes() { + static $suffixes = [ + 'y' => 1e-24, + 'z' => 1e-21, + 'a' => 1e-18, + 'f' => 1e-15, + 'p' => 1e-12, + 'n' => 1e-9, + 'u' => 1e-6, + 'm' => 1e-3, + 'K' => 1e3, + 'M' => 1e6, + 'G' => 1e9, + 'T' => 1e12, + 'P' => 1e15, + 'E' => 1e18, + 'Z' => 1e21, + 'Y' => 1e24, + ]; + + return $suffixes; +} + function thold_display_to_raw($number, $field_name) { $number = trim($number); @@ -5291,96 +5325,19 @@ function thold_display_to_raw($number, $field_name) { return $number; } - $number = trim(substr($number, 0, -1)); + $number = trim(substr($number, 0, -1)); + $suffixes = thold_unit_suffixes(); - if (!is_numeric($number)) { + if (!is_numeric($number) || !isset($suffixes[$suffix])) { $_SESSION['sess_error_fields'][$field_name] = $field_name; raise_message(3); return false; } - switch($suffix) { - case 'f': - return $number * 1e-15; - - break; - case 'p': - return $number * 1e-9; - - break; - case 'u': - return $number * 1e-6; - - break; - case 'm': - return $number * 1e-3; - - break; - case 'K': - return $number * 1e3; - - break; - case 'M': - return $number * 1e6; - - break; - case 'G': - return $number * 1e9; - - break; - case 'T': - return $number * 1e12; - - break; - case 'P': - return $number * 1e15; - - break; - case 'E': - return $number * 1e18; - - break; - case 'Z': - return $number * 1e21; - - break; - case 'Y': - return $number * 1e24; - - break; - default: - $_SESSION['sess_error_fields'][$field_name] = $field_name; - raise_message(3); - - return false; - } + return $number * $suffixes[$suffix]; } -/** - * thold_display_to_raw - Converts a displayed number to a raw - * numeric value. This function converts number like '100M' - * to the raw number 100,000,000, etc. - * - * Supported Units - * - * Unit Expression - * ---- ------------------------------------- - * f Fermo (10e-12) - * p Pico (10e-9) - * u Micro (10e-6) - * m Milli (10e-3) - * K Killo (10e3) - * M Mega (10e6) - * G Giga (10e9) - * T Terra (10e12) - * P Peta (10e15) - * E Exa (10e18) - * Z Zeta (10e21) - * Y Yota (10e24) - * - * @param mixed $number - */ function thold_raw_to_display($number) { if ($number != '') { $number = trim($number); @@ -5394,42 +5351,26 @@ function thold_raw_to_display($number) { return trim($number); } - if ($number > 0) { - $multiplier = 1; - } else { - $multiplier = -1; - } - - $number = abs($number); - $suffix = ''; - - if ($number > 1) { - $pattern = 'KMGTPEZY'; - $count = 0; + $multiplier = $number > 0 ? 1 : -1; + $number = abs($number); - while ($number >= 1e3) { - $count++; - $number /= 1e3; - } + // The largest scale that still leaves a value of one or more, where the + // empty suffix stands for a scale of one. Factors ascend, so the ratio + // falls monotonically and the last match is the one wanted. + $scales = thold_unit_suffixes(); + $scales = array_slice($scales, 0, 8, true) + ['' => 1.0] + array_slice($scales, 8, null, true); - if ($count > 0) { - $suffix = $pattern[$count - 1]; - } - } else { - $pattern = 'mupf'; - $count = 0; - - while ($number < 1) { - $count++; - $number *= 1e3; - } + $suffix = ''; + $factor = 1.0; - if ($count > 0) { - $suffix = $pattern[$count - 1]; + foreach ($scales as $candidate => $candidate_factor) { + if ($number / $candidate_factor >= 1) { + $suffix = $candidate; + $factor = $candidate_factor; } } - return trim(($number * $multiplier) . $suffix); + return trim((($number / $factor) * $multiplier) . $suffix); } function save_thold() { From 3479dba48c964c2ceee27bdd5972538ffeab4a8e Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Mon, 17 Aug 2026 14:47:13 -0700 Subject: [PATCH 2/4] ci: keep plugin PR integration checks on pinned Cacti --- .github/workflows/plugin-ci-workflow.yml | 9 --------- 1 file changed, 9 deletions(-) diff --git a/.github/workflows/plugin-ci-workflow.yml b/.github/workflows/plugin-ci-workflow.yml index 5e4f3db6..79b16b7b 100644 --- a/.github/workflows/plugin-ci-workflow.yml +++ b/.github/workflows/plugin-ci-workflow.yml @@ -35,21 +35,12 @@ jobs: integration-test: runs-on: ${{ matrix.os }} - # A failure against the pinned release is a real failure. The develop entry - # is advisory: it is how a core regression becomes visible here, but it must - # not turn the plugin's own pull requests red. - continue-on-error: ${{ matrix.cacti != 'release/1.2.31' }} - strategy: fail-fast: false matrix: php: ['8.1', '8.2', '8.3', '8.4'] os: [ubuntu-latest] cacti: ['release/1.2.31'] - include: - - php: '8.4' - os: ubuntu-latest - cacti: 'develop' services: mariadb: From ce8ff7f42c9f1391ed08bfe4dcda14d682877f07 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Mon, 17 Aug 2026 14:51:58 -0700 Subject: [PATCH 3/4] test: cover every suffix and isolate session state --- tests/TestCase.php | 1 + tests/Unit/TholdUnitSuffixTest.php | 3 +++ thold_functions.php | 3 ++- 3 files changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/TestCase.php b/tests/TestCase.php index 67342561..a4b4151a 100644 --- a/tests/TestCase.php +++ b/tests/TestCase.php @@ -31,6 +31,7 @@ protected function setUp(): void { CactiStubs::reset(); $GLOBALS['rpn_error'] = false; + $_SESSION = []; } /** diff --git a/tests/Unit/TholdUnitSuffixTest.php b/tests/Unit/TholdUnitSuffixTest.php index 9627a527..801373de 100644 --- a/tests/Unit/TholdUnitSuffixTest.php +++ b/tests/Unit/TholdUnitSuffixTest.php @@ -35,6 +35,9 @@ public static function setUpBeforeClass(): void { */ public static function suffixProvider() { return [ + 'yocto' => ['5y', 5.0e-24], + 'zepto' => ['5z', 5.0e-21], + 'atto' => ['5a', 5.0e-18], 'femto' => ['5f', 5.0e-15], 'pico' => ['5p', 5.0e-12], 'nano' => ['5n', 5.0e-9], diff --git a/thold_functions.php b/thold_functions.php index 6d25c94c..60b2b4c2 100644 --- a/thold_functions.php +++ b/thold_functions.php @@ -5358,7 +5358,8 @@ function thold_raw_to_display($number) { // empty suffix stands for a scale of one. Factors ascend, so the ratio // falls monotonically and the last match is the one wanted. $scales = thold_unit_suffixes(); - $scales = array_slice($scales, 0, 8, true) + ['' => 1.0] + array_slice($scales, 8, null, true); + $scales[''] = 1.0; + asort($scales, SORT_NUMERIC); $suffix = ''; $factor = 1.0; From 38352d21404458c2150420a0865d27286a44f4c3 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Mon, 17 Aug 2026 14:59:50 -0700 Subject: [PATCH 4/4] ci: bound package index refreshes --- .github/workflows/plugin-ci-workflow.yml | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/.github/workflows/plugin-ci-workflow.yml b/.github/workflows/plugin-ci-workflow.yml index 79b16b7b..e17554e8 100644 --- a/.github/workflows/plugin-ci-workflow.yml +++ b/.github/workflows/plugin-ci-workflow.yml @@ -86,7 +86,24 @@ jobs: echo "PHP_BINARY=$(command -v php)" >> "$GITHUB_ENV" - name: Run apt-get update - run: sudo apt-get update + run: | + for attempt in 1 2 3; do + if sudo timeout 3m apt-get \ + -o Dpkg::Lock::Timeout=60 \ + -o Acquire::Retries=3 \ + -o Acquire::http::Timeout=30 \ + -o Acquire::https::Timeout=30 \ + update; then + exit 0 + fi + + if [ "$attempt" -lt 3 ]; then + sleep 10 + fi + done + + echo 'apt-get update failed after three bounded attempts.' >&2 + exit 1 - name: Install System Dependencies run: sudo apt-get install -y apache2 snmp snmpd rrdtool fping