test: add cohesive MacTrack suite - #368
Merged
Merged
Conversation
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
…oor) 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.
TheWitness
requested review from
bmfmancini and
xmacan
and
a lite review from Copilot
September 20, 2026 14:23
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.
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Restore PHP 7.4 compatibility coverage, ensure class-based tests run through the configured runner, and retain equivalent E2E CI coverage.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
Open (3)
What changed in this PR
Adds a Pest-based MacTrack test suite and hardens concurrent Default-site initialization with locking, idempotency, and retry backoff.
Changes:
- Replaces standalone tests with organized unit, security, and integration coverage.
- Updates lifecycle handling for Default-site seeding.
- Updates CI and test configuration.
| File | Summary |
|---|---|
tests/Unit/XformMacAddressTest.php |
Adds MAC transformation coverage. |
tests/Unit/test_prepared_statement_consistency.php |
Retains prepared-statement checks. |
tests/Unit/test_php74_compatibility.php |
Retains legacy compatibility coverage. |
tests/Unit/test_net_dns2_precedence.php |
Tests Net_DNS2 precedence. |
tests/Unit/test_net_dns2_cache_security.php |
Tests cache deserialization safety. |
tests/Unit/test_mac_formatting.php |
Tests MAC formatting. |
tests/Unit/test_ignore_ports_pattern.php |
Tests ignore-port validation. |
tests/Unit/test_filter_option_escaping.php |
Tests filter escaping. |
tests/Unit/test_device_type_sql_safety.php |
Tests device-type SQL safety. |
tests/Unit/MacFormattingTest.php |
Adds framework-based MAC tests. |
tests/Unit/IgnorePortsPatternTest.php |
Adds framework-based port-pattern tests. |
tests/Unit/DefaultSiteSeedingTest.php |
Tests Default-site seeding and retries. |
tests/TestCase.php |
Adds shared test base class. |
tests/Support/CactiStubs.php |
Provides Cacti test stubs. |
tests/Security/SqlSafetyAndOutputEscapingTest.php |
Tests SQL safety and output escaping. |
tests/Security/SetupStructureTest.php |
Tests setup structure. |
tests/Security/PreparedStatementConsistencyTest.php |
Tests prepared-statement consistency. |
tests/Security/Php82CompatibilityTest.php |
Adds PHP 8.2 compatibility checks. |
tests/Security/NetDns2SecurityTest.php |
Tests Net_DNS2 security. |
tests/Pest.php |
Configures Pest. |
tests/Integration/test_mactrack_filter_output_wiring.php |
Tests filter output wiring. |
tests/Integration/FilterOutputWiringTest.php |
Adds framework-based wiring tests. |
tests/Integration/DefaultSiteIdempotencyTest.php |
Tests Default-site idempotency. |
tests/e2e/test_mactrack_no_raw_filter_labels.php |
Removes legacy E2E coverage. |
tests/e2e/run-mactrack-e2e.sh |
Removes legacy E2E runner. |
tests/e2e/mactrack_smoke.php |
Removes legacy smoke test. |
tests/e2e/Dockerfile |
Removes legacy E2E container. |
tests/e2e/docker-compose.yml |
Removes legacy E2E orchestration. |
tests/e2e/bootstrap-mactrack.sh |
Removes legacy E2E bootstrap. |
tests/bootstrap-unit.php |
Adds unit-test bootstrap. |
tests/.cacti-version |
Defines the tested Cacti version. |
setup.php |
Integrates Default-site setup and retry handling. |
poller_mactrack.php |
Retries failed site initialization. |
phpunit.xml |
Configures test execution. |
includes/database.php |
Adds locked, idempotent seeding and backoff. |
CHANGELOG.md |
Documents the Default-site fix. |
.github/workflows/plugin-ci-workflow.yml |
Updates CI matrix and test execution. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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'.
- 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.
bmfmancini
previously approved these changes
Sep 20, 2026
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.
…ompatibility 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.
bmfmancini
approved these changes
Sep 20, 2026
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.
Recreated from #361 (by @somethingwithproof / Thomas Vincent) directly against
Cacti/plugin_mactrack. That PR's branch had drifted significantly behinddevelop(which had since gained its own CI workflow and test-suite updates) and carried a large, non-standard test framework; this PR ports the real fix forward and replaces the test suite with the shared Cacti plugin framework.Attribution
The Default-site concurrency/idempotency fix (issue#357) below is Thomas Vincent's work from #361, carried forward onto current
develop:mactrack_setup_database()/mactrack_database_upgrade()guaranteed a Default site with a racy check-then-insert ("if no rows, INSERT"), so two workers initializing at once (install + poller, concurrent web requests) could both pass the check and insert duplicate Default sites.mactrack_ensure_default_site(), which takes a database-scopedGET_LOCK()advisory lock before a conditionalINSERT ... SELECT ... WHERE NOT EXISTS, verifies the site actually exists afterward rather than trusting the insert's own result, and tracks failed attempts with backoff (60s/5m/15m/30m/1h) viamt_default_site_seed_*config options instead of throwing and leaving the plugin partially registered. Retried fromplugin_mactrack_check_config()and the poller.plugin_mactrack_install()/mactrack_setup_table_new()take an$operator_initiatedflag 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.Not carried forward from #361 (both superseded by
develophistory since that PR was opened): the Composer-managedmikepultz/netdns2migration (reverted by #345, which shipped Net_DNS2 with the plugin again) and themactrack.css/JS path reversion (moved intocss//js/by #363).Test suite
Replaces the plugin's ad-hoc snake_case standalone-script test suite (and #361's own custom Composer/Pest-split harness) 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, andphpunit.xml, run through.github/workflows/plugin-ci-workflow.yml(PHP 8.2-8.4,COMPOSER_ALLOW_SUPERUSER,sudo composerthroughout, SHA-pinned actions).Reorganized into
tests/Security,tests/Unit,tests/Integrationwith PascalCase Pest/PHPUnit files, converting and re-verifying the plugin's existing regression coverage (PHP 8.2 compatibility, prepared-statement ratchet, SQL-safety/output-escaping locks-in, Net_DNS2 cache-deserialization safety, MAC formatting/transform, ignore-ports pattern validation) plus new coverage for the Default-site seeding/retry state machine above.Checklist
php -lclean across the whole plugin treephpunit.xmlvalidated as well-formed XML