Update Testing Framework Consistent with Thold - #29
Merged
TheWitness merged 19 commits intoSep 12, 2026
Merged
Conversation
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Add source-scan tests verifying security patterns (prepared statements, output escaping, auth guards, PHP 7.4 compatibility) remain in place across refactors. Tests run with Pest v1 (PHP 7.3+) and stub the Cacti framework so plugins can be tested in isolation. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
…dabot - Throw RuntimeException when realpath/file_get_contents fails (previously silent continue hid unscanned files) - Fix Dependabot ecosystem from npm to composer - Remove committed .omc session artifacts, add .omc/ to .gitignore Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Align the Evidence plugin's test infrastructure and CI with the model used by Cacti/plugin_thold, while keeping Pest as the test runner: - Remove composer.json/composer.lock: Pest/PHPUnit no longer come from a vendor tree local to this plugin. - Add tests/.cacti-version, tests/TestCase.php, and tests/bootstrap-unit.php (adapted from thold). bootstrap-unit.php verifies the Cacti checkout in CI matches tests/.cacti-version, requires Cacti's own Composer vendor autoloader, and stubs the Cacti global functions plugin source expects. - Replace tests/bootstrap.php with tests/bootstrap-unit.php and update tests/Pest.php's comment to match. - Update phpunit.xml to bootstrap from tests/bootstrap-unit.php, scope the testsuite to tests/Security, and declare covered source files. - Add .github/copilot-instructions.md adapted from thold's for the Evidence plugin. - Add .github/workflows/php-unit-tests.yml modeled on thold's workflow: checks out this plugin plus a pinned Cacti runtime and test toolchain, builds a Docker test image, lints, and runs Pest with coverage.
TheWitness
requested review from
bmfmancini,
browniebraun,
somethingwithproof and
xmacan
September 11, 2026 21:12
bmfmancini
previously approved these changes
Sep 11, 2026
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved issues affect PHPUnit configuration, PHP 7.4 coverage, and Cacti bootstrap compatibility.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates Evidence’s testing infrastructure to use Cacti-managed Pest/PHPUnit tooling with pinned CI integration.
Changes:
- Adds Cacti-aware bootstrap, fixtures, and security tests.
- Adds PHPUnit configuration and Docker-based GitHub Actions testing.
- Removes plugin-local Composer dependencies and adds testing guidance.
File summaries
| File | Description |
|---|---|
tests/TestCase.php |
Adds the PHPUnit base fixture. |
tests/Security/SetupStructureTest.php |
Tests plugin structure metadata. |
tests/Security/Php74CompatibilityTest.php |
Checks PHP 7.4 compatibility. |
tests/Pest.php |
Documents the test bootstrap configuration. |
tests/bootstrap-unit.php |
Adds Cacti runtime validation and test stubs. |
tests/.cacti-version |
Pins the Cacti runtime version. |
phpunit.xml |
Configures tests, bootstrap, and coverage. |
.github/workflows/php-unit-tests.yml |
Adds Docker-based Pest CI. |
.github/copilot-instructions.md |
Documents repository testing conventions. |
Review details
Suppressed comments (5)
tests/Security/Php74CompatibilityTest.php:23
- These four text searches do not actually verify PHP 7.4 parse compatibility: union types, attributes,
match, named arguments, constructor property promotion, andmixedare all PHP 8 syntax that these patterns miss. A source file can introduce one and this test will still pass; runphp -lwith a PHP 7.4 interpreter (or use a PHP parser) over the complete source set instead of relying on this allowlist.
it('does not use str_contains (PHP 8.0)', function () use ($files) {
tests/bootstrap-unit.php:255
- The bootstrap defines the other poller verbosity constants but omits
POLLER_VERBOSITY_HIGH, whichinclude/functions.php::plugin_evidence_time_to_run()uses on every call. A class-based test that loads that source through the new helper will therefore throw an undefined-constant error instead of exercising the function; add the Cacti value for this constant.
if (!defined('POLLER_VERBOSITY_MEDIUM')) {
define('POLLER_VERBOSITY_MEDIUM', 3);
}
tests/bootstrap-unit.php:131
- Cacti's
db_fetch_cell()anddb_fetch_cell_prepared()usefalsefor a no-row result, while these test doubles return an empty string. That changes the missing-value contract and can make PHP 8 behavior differ between tests and production when a caller checks the result strictly or passes it to a typed/internal function. Usefalseas the default no-row value.
if (!function_exists('db_fetch_cell_prepared')) {
function db_fetch_cell_prepared($sql, $params = array()) {
return '';
}
tests/bootstrap-unit.php:125
- Cacti's
db_fetch_cell()returnsfalsewhen no row, but this test double returns an empty string. Keeping the stub's sentinel consistent with Cacti is necessary so tests do not exercise a different missing-value contract; returnfalsehere as well.
if (!function_exists('db_fetch_cell')) {
function db_fetch_cell($sql) {
return '';
}
tests/bootstrap-unit.php:263
- The value for
POLLER_VERBOSITY_NONEdoes not match Cacti: Cacti usesNONE = 1and reserves6forPOLLER_VERBOSITY_DEVDBG. A test that exercises logging or compares verbosity levels will therefore observe the wrong production behavior. Define this constant as1(and addDEVDBGseparately if needed).
if (!defined('POLLER_VERBOSITY_NONE')) {
define('POLLER_VERBOSITY_NONE', 6);
}
- Files reviewed: 9/9 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
2 tasks
TheWitness
added a commit
to TheWitness/plugin_evidence
that referenced
this pull request
Sep 11, 2026
Supersedes Cacti#25 ("hardening: migrate uninstall drop statements to prepared" by @somethingwithproof) — this branch carries forward that PR's prepared-statement hardening and test coverage, then updates the test/CI plumbing to match the model used by Cacti/plugin_thold, mirroring the same work already applied in PR Cacti#29: - Remove composer.json (no composer.lock was present): Pest/PHPUnit no longer come from a vendor tree local to this plugin. - Add tests/.cacti-version, tests/TestCase.php, and tests/bootstrap-unit.php (adapted from thold). bootstrap-unit.php verifies the Cacti checkout in CI matches tests/.cacti-version, requires Cacti's own Composer vendor autoloader, and stubs the Cacti global functions plugin source expects. - Replace tests/bootstrap.php with tests/bootstrap-unit.php and update tests/Pest.php's comment to match. - Add phpunit.xml (none existed before) bootstrapping from tests/bootstrap-unit.php and covering tests/Security, tests/Unit, tests/Integration, and tests/E2E. - Add .github/copilot-instructions.md adapted from thold's for the Evidence plugin. - Add .github/workflows/php-unit-tests.yml modeled on thold's workflow: checks out this plugin plus a pinned Cacti runtime and test toolchain, builds a Docker test image, lints, and runs Pest with coverage.
Merged
1 task
TheWitness
added a commit
to TheWitness/plugin_evidence
that referenced
this pull request
Sep 11, 2026
Adding phpunit.xml means these tests actually execute in CI for the first time; two of them had latent bugs that were never caught before: - AuthGuardTest.php checked tests/test_prepared_statements.php (a test helper script) instead of the plugin's real UI entry points (evidence.php, evidence_tab.php), causing a false-positive failure and a risky (zero-assertion) test once no matching line existed. - SetupStructureTest.php regexed setup.php's raw source for `'name' =>` / `'version' =>`, but those keys live in the INFO ini file that setup.php parses at runtime, not in its literal source text. Assert against the parsed INFO file instead, matching PR Cacti#29's approach.
… fix phpunit.xml schema path
bmfmancini
approved these changes
Sep 12, 2026
bmfmancini
approved these changes
Sep 12, 2026
TheWitness
added a commit
that referenced
this pull request
Sep 12, 2026
… framework with Thold (#30) * security: migrate uninstall drop statements to prepared * fix: complete uninstall drop coverage and harden tests * fix: harden evidence uninstall drop path with IF EXISTS * test: expand security test coverage for hardening changes Add targeted tests for prepared statement migration, output escaping, auth guard presence, CSRF token validation, redirect safety, and PHP 7.4 compatibility. Tests use source-scan patterns that verify security invariants without requiring the Cacti database. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix: escape quotes in Pest regex patterns Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix(security): escape evidence filter output * test: align Evidence test framework with Thold model Supersedes #25 ("hardening: migrate uninstall drop statements to prepared" by @somethingwithproof) — this branch carries forward that PR's prepared-statement hardening and test coverage, then updates the test/CI plumbing to match the model used by Cacti/plugin_thold, mirroring the same work already applied in PR #29: - Remove composer.json (no composer.lock was present): Pest/PHPUnit no longer come from a vendor tree local to this plugin. - Add tests/.cacti-version, tests/TestCase.php, and tests/bootstrap-unit.php (adapted from thold). bootstrap-unit.php verifies the Cacti checkout in CI matches tests/.cacti-version, requires Cacti's own Composer vendor autoloader, and stubs the Cacti global functions plugin source expects. - Replace tests/bootstrap.php with tests/bootstrap-unit.php and update tests/Pest.php's comment to match. - Add phpunit.xml (none existed before) bootstrapping from tests/bootstrap-unit.php and covering tests/Security, tests/Unit, tests/Integration, and tests/E2E. - Add .github/copilot-instructions.md adapted from thold's for the Evidence plugin. - Add .github/workflows/php-unit-tests.yml modeled on thold's workflow: checks out this plugin plus a pinned Cacti runtime and test toolchain, builds a Docker test image, lints, and runs Pest with coverage. * fix(tests): correct pre-existing test bugs surfaced by new CI run Adding phpunit.xml means these tests actually execute in CI for the first time; two of them had latent bugs that were never caught before: - AuthGuardTest.php checked tests/test_prepared_statements.php (a test helper script) instead of the plugin's real UI entry points (evidence.php, evidence_tab.php), causing a false-positive failure and a risky (zero-assertion) test once no matching line existed. - SetupStructureTest.php regexed setup.php's raw source for `'name' =>` / `'version' =>`, but those keys live in the INFO ini file that setup.php parses at runtime, not in its literal source text. Assert against the parsed INFO file instead, matching PR #29's approach. * chore: normalize line endings to LF and enforce via .gitattributes .github/copilot-instructions.md, .github/workflows/php-unit-tests.yml, tests/.cacti-version, tests/TestCase.php, and tests/bootstrap-unit.php were committed with CRLF line endings. Normalize them to LF to match the rest of the repo, and add .gitattributes (`* text=auto eol=lf`) so this doesn't regress. * fix(tests): address Copilot review feedback on PR #30 - PreparedStatementConsistencyTest.php: drop the non-existent tests/test_prepared_statements.php target and narrow the description/ scope to the setup.php uninstall/drop path it actually verifies, instead of implying (and failing to enforce) coverage of every plugin file's DB calls. - RedirectSafetyTest.php: scan the real redirect entry points (index.php, images/index.php, data/index.php) instead of setup.php, which has no header(Location) call at all. - index.php, images/index.php, data/index.php: follow the header("Location: ...") redirect with exit, so the test above passes for a real reason and the scripts can't fall through if code is later added after the redirect. - Php74CompatibilityTest.php: drop the dangling reference to the removed tests/test_prepared_statements.php. - Convert tests/test_prepared_statements.php (a standalone script never discovered by phpunit.xml or Pest's *Test.php convention, so its assertions never ran in CI) into tests/Security/UninstallPreparedStatementsTest.php, a proper Pest test with the same assertions. * chore: remove php-unit-tests.yml workflow from this PR * docs: merge copilot-instructions.md with main's structure * docs: bring in CHANGELOG.md and updated README.md from main * fix(tests): align phpunit.xml schema path and cacti version check with main * docs: clarify renaming plugin_evidence directory to evidence on install * ci: add plugin-ci-workflow.yml from main and run full test suite; docs: raise PHP floor to 8.2 * ci: run composer install without sudo to avoid root-owned vendor tree breaking Pest cache writes --------- Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> Co-authored-by: Thomas Vincent <thomasvincent@gmail.com>
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
This picks up PR #27 ("test: add Pest v1 security test infrastructure") and
updates its testing framework to be consistent with the model used by
Cacti/plugin_thold, while keeping
Pest as the test runner:
composer.json/composer.lock— Pest/PHPUnit no longer come froma vendor tree local to this plugin.
tests/.cacti-version,tests/TestCase.php, andtests/bootstrap-unit.php(adapted from thold).bootstrap-unit.phpverifies the Cacti checkout in CI matches
tests/.cacti-version, requiresCacti's own Composer vendor autoloader, and stubs the Cacti global
functions plugin source expects.
tests/bootstrap.phpwithtests/bootstrap-unit.phpand updatetests/Pest.php's comment to match.phpunit.xmlto bootstrap fromtests/bootstrap-unit.php, scopethe testsuite to
tests/Security, and declare covered source files..github/copilot-instructions.mdadapted from thold's for theEvidence plugin.
Test plan
Pest Tests) builds the Cacti test image and runs Pestagainst
tests/Securitysuccessfully.