Repository navigation
test: add cohesive MacTrack suite - #361
Closed
somethingwithproof wants to merge 4 commits into
Closed
somethingwithproof wants to merge 4 commits into
somethingwithproof wants to merge 4 commits into
Conversation
somethingwithproof
added
bug
QA
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The E2E runner’s DB readiness probe hard-codes credentials and can fail when DB_USER/DB_PASSWORD overrides are used, despite docker-compose supporting those overrides.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR introduces a cohesive, Composer-optional MacTrack test harness (unit/integration/security/static-e2e) and strengthens production lifecycle safety around Default-site seeding so database setup becomes idempotent and concurrency-safe.
Changes:
- Add
tests/run.phpstandalone runner with inventory/fail-closed behavior plus supporting utilities (process runner, CLI guards, Git-tracked PHP manifesting, SQL-call analysis/ratchets, PHP 7.4 scanning). - Add/expand integration and Docker E2E coverage (clean Cacti 1.2.31 install, schema manifest verification, schema idempotency, scanning-function rebuild, concurrent Default-site seeding).
- Update production Default-site handling: replace unconditional inserts with advisory-lock + conditional insert + postcondition verification, plus backoff-based retry and lifecycle-safe reporting in CLI/web/poller paths.
File summaries
| File | Description |
|---|---|
.gitattributes |
Excludes tests/ from release archives via export-ignore. |
.github/workflows/test-suite.yml |
Adds CI workflow to run the cohesive suite across PHP versions plus Docker clean-install lanes. |
CHANGELOG.md |
Notes fix for #357 (Default site duplication on rerun). |
README.md |
Documents new standalone runner and Docker clean-install test path. |
composer.json |
Replaces Pest-based scripts with php tests/run.php ...; aligns platform to PHP 7.4. |
composer.lock |
Updates lockfile to reflect removal of Pest/PHPUnit dev dependencies and platform override. |
includes/database.php |
Implements idempotent/concurrency-safe Default-site seeding with advisory lock + retry/backoff helpers; seeds after full schema setup. |
poller_mactrack.php |
Invokes scheduled Default-site recovery (mactrack_retry_default_site) from the poller. |
phpunit.xml.dist |
Removes PHPUnit configuration (suite no longer runs via phpunit/pest). |
setup.php |
Propagates setup failure in CLI, uses prepared statements for plugin_config update, and triggers Default-site retry in upgrade checks. |
tests/.htaccess |
Denies web access to the tests/ tree. |
tests/Integration/test_default_site_idempotency.php |
Adds integration coverage for Default-site seeding idempotency, lock behavior, and backoff state machine. |
tests/Integration/test_mactrack_filter_output_wiring.php |
Adds CLI guard and improves assertion accounting/output. |
tests/Integration/test_schema_definition.php |
Verifies schema builder creates exactly the documented table set and critical schema elements. |
tests/Pest/E2E/StaticEndpointSafetyTest.php |
Removes obsolete Pest wrapper test. |
tests/Pest/Integration/FilterOutputWiringTest.php |
Removes obsolete Pest wrapper test. |
tests/Pest/Unit/SecurityRegressionTest.php |
Removes obsolete Pest wrapper test. |
tests/Pest/Unit/XformMacAddressTest.php |
Removes Pest-based unit tests superseded by standalone suite. |
tests/Security/Php74CompatibilityTest.php |
Replaces Pest tests with tracked-file PHP 7.4 scanner + php -l parse gate. |
tests/Security/PreparedStatementConsistencyTest.php |
Replaces simple “no raw DB calls” checks with SQL-call baselines/ratchets over tracked production PHP. |
tests/Security/SetupStructureTest.php |
Replaces Pest structure tests with standalone contract checks for setup.php. |
tests/Security/WebExposureTest.php |
Adds web-exposure gate for test harness (redirecting index, denied access, CLI guard enforcement). |
tests/Security/WorkflowBranchCoverageTest.php |
Ensures cohesive workflow branch coverage matches other workflows for push/PR events. |
tests/Support/CactiStubs.php |
Adds CLI guard and adjusts stubs to better match expected Cacti API signatures. |
tests/Support/CliGuard.php |
Adds token-based guard validator ensuring CLI-only tests exit early in web context. |
tests/Support/E2eDatabaseGuard.php |
Adds guard to prevent destructive DB operations outside disposable E2E DB naming. |
tests/Support/Php74Scanner.php |
Adds tokenizer-based detection of PHP 8+ functions/syntax tokens. |
tests/Support/ProcessRunner.php |
Adds a fail-closed process runner with safe environment handling (incl. Git env unsetting). |
tests/Support/ProductionPhpManifest.php |
Adds explicit production PHP manifest for tracked-file validation. |
tests/Support/SchemaManifest.php |
Adds schema manifest for table/column/index verification in tests. |
tests/Support/SqlCallAnalyzer.php |
Adds token-based SQL call analyzer to count raw/dynamic/prepared patterns. |
tests/Support/StandaloneTest.php |
Adds minimal assertion harness used by standalone test runner. |
tests/Support/TestInventory.php |
Adds inventory helper to enforce “no unclaimed PHP test files”. |
tests/Support/TrackedPhpFiles.php |
Adds Git-tracked PHP enumerator with fail-closed semantics when Git is unavailable/mismatched. |
tests/Unit/test_device_type_sql_safety.php |
Adds CLI guard and improves output to report assertion count. |
tests/Unit/test_e2e_database_guard.php |
Adds unit coverage for E2E DB disposable guard matching rules. |
tests/Unit/test_filter_option_escaping.php |
Removes old standalone XSS escaping check (covered elsewhere now). |
tests/Unit/test_mactrack_functions.php |
Adds standalone unit coverage for key production helper functions and key poller wiring expectations. |
tests/Unit/test_runner_fail_closed.php |
Adds runner self-test coverage for empty groups, silent tests, large stderr, and warnings. |
tests/Unit/test_setup_failure_propagation.php |
Adds unit coverage for setup failure propagation (CLI/web behaviors, backoff, logging, metadata writes). |
tests/Unit/test_sql_call_analyzer.php |
Adds unit coverage validating SQL analyzer behavior and error handling. |
tests/Unit/test_tracked_php_files.php |
Adds unit coverage asserting tracked PHP inventory/manifest behavior and fail-closed behavior without Git. |
tests/bootstrap.php |
Expands stubs and state capture for standalone testing (DB calls, config options, logs/messages). |
tests/e2e/bootstrap-mactrack.sh |
Adjusts clean-install bootstrap: imports cacti.sql via mysql client and runs destructive tests with guard validation. |
tests/e2e/docker-compose.yml |
Parameterizes DB image; uses disposable default DB name; updates healthcheck to mysqladmin ping. |
tests/e2e/mactrack_concurrent_default_site.php |
Adds destructive E2E concurrency/held-lock tests for Default-site seeding. |
tests/e2e/mactrack_schema_idempotency.php |
Adds destructive E2E test for schema setup idempotency and no Default resurrection. |
tests/e2e/mactrack_scanning_functions.php |
Adds destructive E2E test asserting scanning-function rebuild is idempotent. |
tests/e2e/mactrack_smoke.php |
Expands smoke test to validate schema manifest, core helper contracts, and Default seeded exactly once. |
tests/e2e/run-mactrack-e2e.sh |
Updates DB readiness probing and adds fail-closed timeout behavior before running bootstrap/smoke. |
tests/e2e/test_mactrack_no_raw_filter_labels.php |
Adds CLI guard and improves assertion accounting/output. |
tests/fixtures/inventory/Security/ClaimedTest.php |
Adds inventory fixture file for runner inventory testing. |
tests/fixtures/inventory/Security/test_orphan.php |
Adds orphan fixture file for inventory mismatch detection testing. |
tests/fixtures/inventory/Unit/OrphanTest.php |
Adds orphan fixture file for inventory mismatch detection testing. |
tests/fixtures/inventory/Unit/test_claimed.php |
Adds inventory fixture file for runner inventory testing. |
tests/fixtures/no-tests/.gitkeep |
Adds empty-dir keeper for inventory/runner fixtures. |
tests/fixtures/test_large_stderr.php |
Adds runner fixture emitting large stderr to validate runner doesn’t deadlock and fails actionable. |
tests/fixtures/test_noop.php |
Adds runner fixture to validate basic success reporting. |
tests/fixtures/test_silent.php |
Adds runner fixture to validate fail-closed on “no assertions completed” output. |
tests/fixtures/test_warning.php |
Adds runner fixture to validate fail-closed on stderr warnings even with 0 exit code. |
tests/index.php |
Redirects web requests away from tests (replaces Pest bootstrap config). |
tests/run.php |
Adds Composer-independent test entrypoint with group selection, inventory enforcement, and fail-closed execution rules. |
Review details
- Files reviewed: 61/62 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
xmacan
approved these changes
Sep 15, 2026
3 tasks done
Member
|
Superseded by #368. This branch had drifted significantly behind Thank you for the fix and the extensive test coverage here — closing this in favor of #368. |
TheWitness
added a commit
that referenced
this pull request
Sep 20, 2026
* fix: prevent repeated database setup from duplicating the Default site Ports the Default-site concurrency/idempotency fix from PR #361 (by Thomas Vincent / somethingwithproof) forward onto current develop. mactrack_setup_database()/mactrack_database_upgrade() used a racy check-then-insert ('if no rows, INSERT') to guarantee a Default site existed, so two workers initializing at once (install + poller, concurrent web requests) could both pass the check and insert duplicate Default sites. Adds mactrack_ensure_default_site(), which: - takes a database-scoped GET_LOCK() advisory lock before the conditional INSERT ... SELECT ... WHERE NOT EXISTS, closing most of the race (durable name-level uniqueness is tracked separately in #360, since a reconnect can release the advisory lock) - verifies the site actually exists after seeding/after failing to acquire the lock, rather than trusting the insert's own result - tracks failed attempts with backoff (60s/5m/15m/30m/1h) via mt_default_site_seed_* config options, retried from plugin_mactrack_check_config() and the poller, instead of throwing and leaving the plugin partially registered plugin_mactrack_install()/mactrack_setup_table_new() take an flag so an operator-triggered (re)install always clears prior backoff state, while an automatic upgrade check preserves it. plugin_mactrack_uninstall() cleans up the tracking settings. Also hardens mactrack_check_upgrade()'s plugin_config UPDATE to use prepared statements. Closes #357 * test: adopt the shared Cacti plugin test framework (Pest + PHP 8.2 floor) Replaces the plugin's ad-hoc snake_case standalone-script test suite and its custom .github/workflows/test-suite.yml with the framework used across other Cacti plugins (modeled on plugin_evidence): Pest via Cacti's own Composer-managed vendor tree, tests/bootstrap-unit.php, tests/TestCase.php, tests/Pest.php, tests/.cacti-version, and phpunit.xml, run through .github/workflows/plugin-ci-workflow.yml (PHP 8.2-8.4, CACTI/COMPOSER_ALLOW_SUPERUSER env vars, sudo composer throughout, SHA-pinned actions). Removes tests/Support/CactiStubs.php, the tests/e2e/ Docker harness, and every snake_case test_*.php file, reorganizing coverage into tests/Security, tests/Unit, and tests/Integration with PascalCase Pest files: - Security/Php82CompatibilityTest.php: renamed and adapted from test_php74_compatibility.php now that the plugin's floor is PHP 8.2, scanning for 8.3/8.4-only syntax instead of 8.0+ syntax. - Security/PreparedStatementConsistencyTest.php: the raw-SQL-call ratchet from test_prepared_statement_consistency.php, rebaselined against current source. - Security/SetupStructureTest.php: standard hook/realm/INFO structural checks, new to this plugin. - Security/NetDns2SecurityTest.php: converted from test_net_dns2_cache_security.php + test_net_dns2_precedence.php. - Security/SqlSafetyAndOutputEscapingTest.php: converted from test_device_type_sql_safety.php, re-verified against current source. - Unit/MacFormattingTest.php, Unit/XformMacAddressTest.php: converted from test_mac_formatting.php and rewritten as PHPUnit data-provider tests covering xform_mac_address()'s ASCII/HEX-/binary paths. - Unit/IgnorePortsPatternTest.php: converted from test_ignore_ports_pattern.php. - Unit/DefaultSiteSeedingTest.php, Integration/DefaultSiteIdempotencyTest.php: new coverage for the mactrack_ensure_default_site()/ mactrack_seed_default_site() advisory-lock seeding and retry/backoff state machine added in the prior commit (issue#357), including the install/upgrade entry points that call it. - Integration/FilterOutputWiringTest.php: converted from test_mactrack_filter_output_wiring.php. tests/bootstrap-unit.php extends the plugin_evidence model with an in-memory config-option store (mactrack's retry state lives entirely in read_config_option()/set_config_option()) and CactiStubs-style SQL-fragment-matched return values for db_fetch_cell_prepared(), needed to exercise the GET_LOCK/RELEASE_LOCK advisory-locking path. * ci: remove the legacy code-quality.yml workflow Duplicated plugin-ci-workflow.yml's PHP lint/PHPStan/coding-standards steps, and its 'Run standalone tests' step assumed every file in tests/Unit and tests/Integration was directly php-executable, which was true of the old snake_case standalone-script test suite but is not true of the PHPUnit/Pest class-based tests that replaced it. No other Cacti plugin (plugin_evidence, plugin_thold, etc.) carries a separate code-quality.yml alongside plugin-ci-workflow.yml. * fix: set the Net_DNS2 include path before requiring it in tests Net_DNS2.php's own internal requires are include-path relative (same reason mactrack_resolver.php calls set_include_path() before requiring it in production). The test required it directly, which happened to work when run standalone from the plugin directory but failed under Pest, which runs from Cacti's root: 'Class Net_DNS2_Cache not found'. * test: fix CI failures in the new Pest suite - Php82CompatibilityTest.php: dropped the 'parses under the running PHP version' check; it duplicates the CI workflow's own dedicated PHP syntax-check step and its shell_exec()-based assertion behaved inconsistently under Pest. - IgnorePortsPatternTest.php: the invalid-pattern cases deliberately feed malformed regex to mactrack_validate_ignore_ports_pattern(), which already suppresses the resulting preg_match() compilation warning with @, but phpunit.xml's failOnWarning="true" still turned it into a test failure. Suspend the warning handler for the duration of that specific call instead. * test: restore PHP 7.4 compatibility regression coverage Copilot review flagged that renaming test_php74_compatibility.php to Php82CompatibilityTest.php dropped the plugin's documented PHP 7.4 support contract (README.md, .github/copilot-instructions.md) without a replacement guard, so a future 8.0+-only construct could pass CI undetected. Restore Php74CompatibilityTest.php with equivalent coverage to the original standalone script (str_contains/str_starts_with/str_ends_with, nullsafe operator) plus a match-expression check, verified to have no false positives against the current production tree. Php82CompatibilityTest.php is kept as-is; it targets syntax that would break the newer legs of the CI matrix (8.3/8.4), which is a distinct concern from the 7.4 production support floor. * docs: fix the PHP floor documentation back to 8.2, drop the PHP 7.4 compatibility test d6639da restored tests/Security/Php74CompatibilityTest.php because README.md and .github/copilot-instructions.md said the plugin's production-code floor was PHP 7.4 - but that floor was itself wrong; this repo's CI, Php82CompatibilityTest.php, and every other plugin using this shared test model already target PHP 8.2. Fix the actual source of the confusion instead of accommodating it: - README.md, .github/copilot-instructions.md: PHP floor documented as 8.2 everywhere, matching the CI matrix and Php82CompatibilityTest.php. - Remove tests/Security/Php74CompatibilityTest.php: it enforced avoiding str_contains()/str_starts_with()/str_ends_with()/the nullsafe operator, all of which are fine on an 8.2 floor and already used elsewhere in the plugin; keeping it would fail CI on legitimate, floor-compatible code. - Drop the now-dangling Php74CompatibilityTest.php cross-references from Php82CompatibilityTest.php's docblock/comment. - CHANGELOG.md: correct the 'docs: Align...' entry to describe the 8.2 alignment instead of the 7.4 one.
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.
Summary
Closes #357
Related to #355
Related to #356
Validation
sprintf(),str_replace(), helper calls, and method callscomposer testandcomposer validate --strictactionlint .github/workflows/test-suite.ymlshellcheck tests/e2e/*.shgit diff --checkCoordination
falseresult; MacTrack therefore reports the incomplete Default-site setup and retries safely from the poller instead of risking a partially registered plugin by throwing.