Skip to content

feat(test): expand unit test infrastructure and coverage#7123

Closed
somethingwithproof wants to merge 4 commits into
Cacti:developfrom
somethingwithproof:feat/unit-test-expansion-infra
Closed

feat(test): expand unit test infrastructure and coverage#7123
somethingwithproof wants to merge 4 commits into
Cacti:developfrom
somethingwithproof:feat/unit-test-expansion-infra

Conversation

@somethingwithproof
Copy link
Copy Markdown
Contributor

@somethingwithproof somethingwithproof commented May 8, 2026

Modernizes the test runner, adds PSR-4 infrastructure for new code under Cacti\ namespace, lands review fixes for the security-sensitive surface, and adds a substantive database-layer unit test suite.

Scope

  • Upgrade Pest to 3.x and configure PSR-4 autoload for Cacti\ (lib/).
  • Reorganize new infrastructure under lib/Security, lib/Http, lib/Log, lib/Process, lib/Filesystem.
  • Add symfony/validator, symfony/http-foundation, symfony/process, symfony/filesystem, psr/log.
  • Add Cacti\Security\CactiOAuth provider factory; lib/functions.php and oauth2.php now route OAuth construction through it.

Review fixes

  • CactiValidator::isValidRrdPath now rejects empty input, NUL bytes, absolute paths, and verifies containment under an optional $rraRoot via realpath.
  • CactiValidator::isValidEmail enforces the RFC 5321 254-octet cap.
  • New CactiValidator::isValidIpAddress, isValidEmail, isValidSnmpCommunity helpers.
  • include/global.php test-only DB short-circuit now requires both PHP_TESTING define and CACTI_TEST_BOOTSTRAP=1 env. The DB sentinel is a Cacti_TestDbSentinel object that throws on any method call rather than a truthy boolean.
  • CactiRequest::current() only installs a MockArraySessionStorage when CACTI_TEST_BOOTSTRAP=1; production paths rethrow SessionNotFoundException.
  • CactiRequest::set() no longer writes $_POST; has() consults attributes; isAjax() uses strict equality and is documented as non-authoritative.
  • CactiProcess::split() docblock corrected: best-effort tokenizer; do not pass attacker-influenced input.
  • reset() added to CactiValidator, CactiFilesystem, CactiLogger singletons. beforeEach resets each in their tests.
  • CactiRequestTest cleans $_GET/$_POST/$_REQUEST/$_SESSION in afterEach.
  • OrbEnvironmentTest skips when the orb CLI is unavailable; commands run under timeout 30 when available.
  • Pest.php no longer references the missing HandOff directory.

Database layer test suite

  • New tests/Helpers/FakeMySQLPDO.php translates MySQL-specific syntax (SHOW TABLES, SHOW COLUMNS, INSERT ... ON DUPLICATE KEY UPDATE, SHOW INDEXES) so lib/database.php runs unmodified against sqlite::memory:. Test-only; never loaded by production.
  • 5 new test files under tests/Unit/Db*Test.php covering: db_qstr, array_to_sql_or, db_strip_control_chars, db_format_index_create, db_execute, db_execute_prepared, db_fetch_cell_prepared, db_fetch_assoc_prepared, db_affected_rows, db_fetch_insert_id, db_table_exists, db_column_exists, sql_save, db_replace, and db_connect_real failure path.
  • 1 test skipped (documented): db_fetch_row_return reads PDOStatement::rowCount() which the SQLite PDO driver leaves at 0 for SELECT statements; a substitute test exercising the same insert + lookup path via db_fetch_assoc_prepared is included and passes.

Test results

  • 649 passed, 1 skipped (SQLite PDO driver limitation), 0 failed across the unit suite under CACTI_TEST_BOOTSTRAP=1.
  • Cacti\Security\CactiOAuth at 100% line and branch coverage (24/24 lines, 22/22 branches).

Risk

  • OAuth provider construction now flows through a factory. Behavior preserved for google/microsoft/azure/yahoo. Keycloak remains constructable; getDefaultOptions('keycloak') returns [] and callers must supply realm-specific scopes.
  • include/global.php change is gated on both PHP_TESTING define and CACTI_TEST_BOOTSTRAP=1 env; production paths fail closed.
  • FakeMySQLPDO is test infrastructure only and never autoloaded outside the test bootstrap.

Signed-off-by: Thomas Vincent thomasvincent@gmail.com

* Improve IPv6 support in RRDtool proxy and ping utilities

- Fix RRDtool proxy to dynamically detect IPv6 addresses and use
  AF_INET6 sockets instead of hardcoded AF_INET (IPv4-only), enabling
  connections to IPv6 RRDtool proxy servers including backup servers
- Strip brackets from IPv6 addresses before passing to socket_connect
- Properly close failed sockets before creating new ones for failover
- Replace fragile str_contains(':') IPv6 detection in ping with
  filter_var(FILTER_FLAG_IPV6) for more robust address validation

https://claude.ai/code/session_01SbuDigvAkYvPKvdcougsdo

* Fix JS code quality: implicit globals, deprecated APIs, loose equality

- Add var declarations to prevent implicit globals in install.js
  (element, enabled, button, buttonCheck)
- Remove console.log debug output left in production (install.js)
- Replace deprecated jQuery .unbind() with .off() (layout.js)
- Fix "depreciated" typo to "deprecated" in deprecation warnings
- Convert == / != to === / !== for null, boolean, string, typeof,
  and numeric comparisons across install.js and realtime.js

https://claude.ai/code/session_01SbuDigvAkYvPKvdcougsdo

* Replace deprecated jQuery shorthand event methods in layout.js

Replace .click(), .keyup(), .keydown(), .mousedown(), .mouseenter(),
.mouseleave(), .submit(), .resize() with .on() equivalents. Replace
.focus(), .change() trigger calls with .trigger(). These shorthands
were deprecated in jQuery 3.5.

https://claude.ai/code/session_01SbuDigvAkYvPKvdcougsdo

* Replace deprecated jQuery methods in realtime.js

Replace .bind() with .on() and .change() trigger calls with
.trigger('change'). .bind() was deprecated in jQuery 3.0 and
shorthand triggers in jQuery 3.5.

https://claude.ai/code/session_01SbuDigvAkYvPKvdcougsdo

* Replace deprecated jQuery methods in install.js and fix ===/!== errors

Replace .click(), .change(), .focus() with .on()/.trigger() equivalents.
Also fix !=== and ==== operators that were incorrectly introduced by a
prior replace-all of == to === within existing !== and === expressions.

https://claude.ai/code/session_01SbuDigvAkYvPKvdcougsdo

* Restore == null where needed to catch both null and undefined

In JavaScript, == null matches both null and undefined, which is an
intentional idiom. The prior === null conversion broke cases where
values come from jQuery .val(), .data(), $.urlParam(), or object
property access that may return undefined rather than null. Revert
those specific cases while keeping === null where variables are
explicitly initialized to null.

https://claude.ai/code/session_01SbuDigvAkYvPKvdcougsdo

* Replace deprecated jQuery methods in all theme main.js files

Replace .unbind().click() with .off('click').on('click'), convert
.hover() to .on('mouseenter').on('mouseleave'), replace .change(),
.scroll(), .click() shorthands with .on() equivalents, and .blur()
with .trigger('blur') across all 10 theme files.

https://claude.ai/code/session_01SbuDigvAkYvPKvdcougsdo

* Convert string concatenation to template literals

Replace 'str' + var + 'str' patterns with ES6 template literals
in realtime.js and install.js. Improves readability especially for
URL construction and HTML building. Also replace $.parseJSON() with
native JSON.parse() in realtime.js.

https://claude.ai/code/session_01SbuDigvAkYvPKvdcougsdo

* Convert var to const for single-assignment variables

Replace var with const where the variable is assigned once and never
reassigned within its scope, in install.js and realtime.js. Keeps var
for variables that are conditionally reassigned (e.g. size, url).

https://claude.ai/code/session_01SbuDigvAkYvPKvdcougsdo

---------

Co-authored-by: Claude <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings May 8, 2026 04:47
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot wasn't able to review this pull request because it exceeds the maximum number of files (300). Try reducing the number of changed files and requesting a review from Copilot again.

- Upgrade Pest to 3.x
- Configure PSR-4 autoloading for Cacti namespace
- Add missing Symfony dependencies (validator, foundation, process, filesystem)
- Add 29 new tests across 8 suites (validation, time, process, filesystem, logger, request, utility, rrd)
- Harden global.php for database isolation during testing
- Reorganize internal classes into PSR-4 compliant subdirectories

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
…rage

Tighten CactiValidator path traversal, gate test-only DB sentinel on
CACTI_TEST_BOOTSTRAP env, drop singleton state leaks, and skip Orb
integration tests when the orb CLI is unavailable. CactiOAuth provider
factory replaces inline switch in lib/functions.php and oauth2.php.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
@somethingwithproof somethingwithproof force-pushed the feat/unit-test-expansion-infra branch from 101b69a to 785d5ea Compare May 8, 2026 06:26
…ecorator

Cover db_qstr, array_to_sql_or, db_strip_control_chars, db_format_index_create,
db_execute, db_execute_prepared, db_fetch_*_prepared, db_affected_rows,
db_fetch_insert_id, db_table_exists, db_column_exists, sql_save, db_replace,
and db_connect_real failure path. The FakeMySQLPDO test helper translates
SHOW TABLES, SHOW COLUMNS, and INSERT ... ON DUPLICATE KEY UPDATE so
lib/database.php runs unmodified against sqlite::memory:.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
@somethingwithproof
Copy link
Copy Markdown
Contributor Author

Splitting into smaller atomic PRs to keep CI green and reviewable:

PR-A also pins Pest at v2 (downgraded from v3) and regenerates composer.lock
under PHP 8.1 so all four PHP matrix entries (8.1-8.4) install cleanly. The
new lib/* files were run through composer run-script php-cs-fixit before
each split commit. Closing in favor of those.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants