diff --git a/.github/workflows/plugin-ci-workflow.yml b/.github/workflows/plugin-ci-workflow.yml index 5e4f3db6..e17554e8 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: @@ -95,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 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 new file mode 100644 index 00000000..801373de --- /dev/null +++ b/tests/Unit/TholdUnitSuffixTest.php @@ -0,0 +1,176 @@ + + */ + 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], + '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..60b2b4c2 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,27 @@ 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[''] = 1.0; + asort($scales, SORT_NUMERIC); - 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() {