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/Unit/GetCurrentValueTest.php b/tests/Unit/GetCurrentValueTest.php new file mode 100644 index 00000000..f467b9bc --- /dev/null +++ b/tests/Unit/GetCurrentValueTest.php @@ -0,0 +1,148 @@ + 300]); + + // thold_rrd_last() returns whatever `rrdtool last` printed. + CactiStubs::willReturn('rrdtool_execute', '1700000000'); + } + + /** + * @param array $fetch + * + * @return void + */ + private function rrdReturns(array $fetch) { + CactiStubs::willReturn('rrdtool_function_fetch', $fetch); + } + + /** + * @return void + */ + public function testTheRequestedDataSourceValueIsReturned(): void { + $this->rrdReturns([ + 'data_source_names' => ['traffic_in', 'traffic_out'], + 'values' => [['1700000000' => 10.0], ['1700000000' => 20.0]], + ]); + + $this->assertSame(20.0, get_current_value(4, 'traffic_out')); + } + + /** + * array_search() returns false, not null, so a guard written against null + * let the miss through and PHP then read index 0 — the first data source. + * + * @return void + */ + public function testUnknownDataSourceReturnsZeroRatherThanTheFirstOne(): void { + $this->rrdReturns([ + 'data_source_names' => ['traffic_in', 'traffic_out'], + 'values' => [['1700000000' => 10.0], ['1700000000' => 20.0]], + ]); + + $this->assertSame(0, get_current_value(4, 'upper_limit')); + } + + /** + * @return void + */ + public function testFirstDataSourceIsStillReachableByName(): void { + $this->rrdReturns([ + 'data_source_names' => ['traffic_in', 'traffic_out'], + 'values' => [['1700000000' => 10.0], ['1700000000' => 20.0]], + ]); + + $this->assertSame(10.0, get_current_value(4, 'traffic_in')); + } + + /** + * @return void + */ + public function testMissingDataSourceNamesReturnsZero(): void { + $this->rrdReturns([]); + + $this->assertSame(0, get_current_value(4, 'traffic_in')); + } + + /** + * @return void + */ + public function testMissingValuesReturnsZero(): void { + $this->rrdReturns(['data_source_names' => ['traffic_in']]); + + $this->assertSame(0, get_current_value(4, 'traffic_in')); + } + + /** + * @return void + */ + public function testEmptyValueSeriesReturnsZero(): void { + $this->rrdReturns([ + 'data_source_names' => ['traffic_in'], + 'values' => [[]], + ]); + + $this->assertSame(0, get_current_value(4, 'traffic_in')); + } + + /** + * A missing or unreadable RRD makes `rrdtool last` print nothing, which + * used to reach the timestamp arithmetic as an empty string and fatal. + * + * @return void + */ + public function testUnreadableRrdReturnsZeroRatherThanThrowing(): void { + CactiStubs::reset(); + CactiStubs::willReturn('db_fetch_row_prepared', ['rrd_step' => 300]); + CactiStubs::willReturn('rrdtool_execute', ''); + $this->rrdReturns([]); + + $this->assertSame(0, get_current_value(4, 'traffic_in')); + } + + /** + * @return void + */ + public function testValueIsRoundedToFourDecimals(): void { + $this->rrdReturns([ + 'data_source_names' => ['traffic_in'], + 'values' => [['1700000000' => 1.23456789]], + ]); + + $this->assertSame(1.2346, get_current_value(4, 'traffic_in')); + } +} diff --git a/tests/Unit/TholdStrReplaceTest.php b/tests/Unit/TholdStrReplaceTest.php new file mode 100644 index 00000000..69006c39 --- /dev/null +++ b/tests/Unit/TholdStrReplaceTest.php @@ -0,0 +1,94 @@ + + */ + public static function preservedValueProvider() { + return [ + 'integer zero' => [0, 'v=0'], + 'string zero' => ['0', 'v=0'], + 'float zero' => [0.0, 'v=0'], + 'negative' => [-5, 'v=-5'], + 'positive integer' => [5, 'v=5'], + 'float' => [2.5, 'v=2.5'], + 'string zero decimal' => ['0.0', 'v=0.0'], + ]; + } + + /** + * @dataProvider preservedValueProvider + * + * @param mixed $replace + * @param string $expected + * + * @return void + */ + public function testNumericValuesSurviveSubstitution($replace, $expected): void { + $this->assertSame($expected, thold_str_replace('', $replace, 'v=')); + } + + /** + * @return array + */ + public static function absentValueProvider() { + return [ + 'null' => [null], + 'false' => [false], + 'empty string' => [''], + ]; + } + + /** + * @dataProvider absentValueProvider + * + * @param mixed $replace + * + * @return void + */ + public function testAbsentValuesBecomeEmpty($replace): void { + $this->assertSame('v=', thold_str_replace('', $replace, 'v=')); + } + + /** + * @return void + */ + public function testEveryOccurrenceIsReplaced(): void { + $this->assertSame('0 and 0', thold_str_replace('', 0, ' and ')); + } + + /** + * @return void + */ + public function testSubjectWithoutTheTagIsUnchanged(): void { + $this->assertSame('no tags here', thold_str_replace('', 5, 'no tags here')); + } +} diff --git a/tests/bootstrap-unit.php b/tests/bootstrap-unit.php index ce2b2615..41a83a31 100644 --- a/tests/bootstrap-unit.php +++ b/tests/bootstrap-unit.php @@ -157,7 +157,7 @@ function db_fetch_cell_prepared($sql, $params = [], $col_name = '', $log = true, } if (!function_exists('db_qstr')) { - function db_qstr($string) { + function db_qstr($string, $db_conn = false) { return "'" . str_replace("'", "''", (string) $string) . "'"; } } 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..d8228c50 100644 --- a/thold_functions.php +++ b/thold_functions.php @@ -4861,8 +4861,9 @@ function get_current_value($local_data_id, $data_template_rrd_id, $cdef = 0) { $last_time_entry = thold_rrd_last($local_data_id); - // This should fix and 'did you really mean month 899 errors', this is because your RRD has not polled yet - if ($last_time_entry == -1) { + // This should fix and 'did you really mean month 899 errors', this is because your RRD has not polled yet. + // A missing or unreadable RRD makes rrdtool print nothing, which is not a timestamp either. + if (!is_numeric($last_time_entry) || $last_time_entry == -1) { $last_time_entry = time(); } @@ -4884,11 +4885,13 @@ function get_current_value($local_data_id, $data_template_rrd_id, $cdef = 0) { return 0; } + // array_search() reports a miss as false. Testing for null let the miss + // through, and $result['values'][false] then read index 0, so a lookup for + // a data source that does not exist returned the first one's value. $idx = array_search($data_template_rrd_id, $result['data_source_names'], true); // Return Blank if the value was not found (Cache Cleared?) - - if (!isset($result['values']) || $idx === null || !cacti_sizeof($result['values'][$idx])) { + if ($idx === false || !isset($result['values'][$idx]) || !cacti_sizeof($result['values'][$idx])) { return 0; } @@ -8310,12 +8313,19 @@ function thold_get_cached_name(&$thold_data) { return $thold_data['name_cache']; } -function thold_str_replace($search, $replace, $subject) { - if (empty($replace) || $replace === 0) { - $replace = ''; - } - - return str_replace($search, $replace, $subject); +/** + * Substitute one tag, rendering an absent value as an empty string. + * + * Only null and false count as absent. Zero is a legitimate reading, and + * blanking it produced alert bodies reading "Current value is " for exactly + * the case an operator most needs to see. + * + * @param string $search Tag to replace. + * @param mixed $replace Value to substitute. + * @param string $subject Text containing the tag. + */ +function thold_str_replace(string $search, $replace, string $subject): string { + return str_replace($search, $replace ?? '', $subject); } function thold_template_import($xml_data) {