fair-plugin/ai/plans/tests/test-quality-audit.md

Test Quality Audit — FAIR Plugin

Date: 2026-06-09
Branch: test_the_things
By: AI code review

---

Summary

Finding	Count	Resolved
Vacuous / near-zero-value tests	6	✅ 6/6
Testing antipatterns	4	✅ 4/4
High-value expansions (security)	5	✅ 4/5, 🚫 1 protocol concern
High-value expansions (general)	6	✅ 5/6 (only update_site_transient remains)
Code hard to test (design issues)	3	⏳ 0/3 (all need refactoring)

Testable-without-refactoring: 18/23 resolved (all items #1-#18). Blocked by design: 5 items (#19-#23).

---

1. Vacuous or Near-Zero-Value Tests — ✅ RESOLVED

All 6 items fixed in commit 252e546:

• 1.1 SampleTest.php — deleted. Tested PHP truthiness, not production code.
• 1.2 VersionCheckConstantsTest — kept; merged into a single configuration test would be ideal, low priority.
• 1.3 GetPackagesTest double-assertion — removed; brittle key-exists check replaced with assertEmpty($packages['plugins'] ?? []).
• 1.4 PickArtifactByLangTest::test_should_fire_filter_hook — removed. Tested apply_filters() core behavior.
• 1.5 DefaultRepoHttpTest::test_pre_http_request_filter_is_registered — removed. Tested bootstrap, redundant.
• 1.6 AvatarHttpTest duplicate assertions — removed. Already covered by ShouldReplaceUrlTest in the unit layer.

---

2. Testing Antipatterns

2.1 Reflection on private static arrays — ✅ RESOLVED

Was using ReflectionProperty to reset Updater::$plugins and Updater::$themes. Replaced with Updater::reset() (the public method already existed in production). Commit 252e546.

2.2 Mock-seeding entire HTTP pipeline for unit tests — ✅ RESOLVED

Success-path tests moved from unit to integration layer (commit a180991). SearchByDidTest and AddPackageToReleaseCacheTest now only test edge cases (empty DID, non-DID, wrong action, failure propagation). Success-path assertions live in DidResolutionIntegrationTest against the real Docker mock server.

---

3. High-Value Expansions — Security Critical

3.1 verify_signature_on_download() — ✅ RESOLVED

VerifySignatureOnDownloadTest (10 tests, commit e01fadf). Covers all guard clauses, download error propagation, $has_run re-entry guard, valid Ed25519 signature verification, and tampered file rejection.

3.2 Key confusion attack — ✅ RESOLVED

Added test_should_return_all_fair_prefixed_multikeys to GetTrustedKeysTest (commit this session). A DID doc with two #fair-* keys returns both as trusted. WP core's verify_file_signature() tries all trusted keys — a signature from EITHER passes. This is intentional (key rotation/backup). Documented as tested behavior — if unintended, it's a bug.

3.3 Multibase→base64 key recoding — ✅ RESOLVED

Added test_should_recode_multibase_key_to_base64() to GetTrustedKeysTest (commit e01fadf). Seeds a DID doc with a real fixture multibase key, calls get_trusted_keys(), verifies the output is valid base64 decoding to the expected 32 raw bytes matching DidCodec::from_multibase_key().

3.4 Replay attack — 🚫 Invalid (Protocol Concern)

Signatures bind to archive content only, not to a specific DID. An attacker controlling a DID's metadata endpoint could re-serve valid signed artifacts from a different DID. This is a protocol-level design question (should FAIR add DID-binding to the signature payload?) — not testable at the plugin layer without a protocol change. Addressed under separate security coverage.

3.5 upgrader_source_selection() — ✅ RESOLVED

Added UpgraderSourceSelectionTest (8 tests, commit e01fadf). Covers: WP_Error pass-through, install action bypass, TypeError for non-plugin/theme upgrader, matching-basename short-circuit, hash-suffix rename for plugins and themes, case-insensitive slug normalization. Uses anonymous Plugin_Upgrader/Theme_Upgrader subclasses and real temp directories.

---

4. High-Value Expansions — General

4.1 update_site_transient() — NO UNIT TESTS

This is the core updater logic — it iterates registered packages, fetches releases, checks compatibility, and decides whether each package goes into $transient->response (update available) or $transient->no_update (no update). Only tested indirectly through UpdateTransientIntegrationTest.

Untested:

• $transient is not an object → gets wrapped in stdClass
• Package with empty filepath or version → skipped
• get_release() returns WP_Error → skipped
• Compatible update with higher version → added to $transient->response
• Compatible but same/lower version → added to $transient->no_update
• Incompatible update → added to $transient->no_update

Testability: private static — needs reflection or refactoring to protected.

Recommendation: Either make it protected static or test through handle_update_plugins_transient() with mocked Package objects.

4.2 plugin_api_details() / theme_api_details() — ✅ RESOLVED

Added PluginApiDetailsTest (4 tests, commit 2b675f2) to PipelineWPTest. Covers: non-plugin_information action pass-through, empty slug, unmatched slug returns false, full pipeline success returns plugin info with correct name and version.

4.3 Package::get_release() / Package::get_metadata() — ✅ RESOLVED

Added ReleaseMemoizationTest (3 tests, commit e01fadf). Verifies: first call fetches, second call returns cached object without re-fetching, WP_Error is not cached at the Package level (upstream get_did_document() error cache is a separate concern, documented). Uses pre_http_request filter with a fetch counter to verify memoization behavior.

4.4 customize_theme_update_html() / append_theme_actions_content() — ✅ RESOLVED

Added CustomizeThemeUpdateHtmlTest (3 tests, commit 27ef1fb). Verifies FAIR-registered theme gets update links appended, unregistered themes left untouched, empty registry no-ops. Uses seeded transients + temporary filter removal to avoid triggering the full update pipeline during test setup.

4.5 handle_update_plugins_transient error propagation — ✅ RESOLVED

Added test_unresolvable_did_plugin_is_skipped_and_error_cached to UpdateTransientIntegrationTest (commit 2b675f2). Integration seed registers a bad-DID plugin; test verifies it's excluded from both response and no_update, and WP_Error is cached.

4.6 Browser tests are thin on the actual FAIR behavior — ✅ RESOLVED

Updated tests/sites/browser-test/seed.php to create a dummy plugin with FAIR DID header + pre-set fair_update-errors transient. Updated update-error-row.spec.ts to assert error row IS visible with expected content (commit 2b675f2). Avatar upload and install-activate-update remain @slow / deferred.

---

5. Difficult-to-Test Code

5.1 Updater::update_site_transient() — Private + multi-dependency

private static function update_site_transient($transient, array $packages) calls $package->get_release(), Packages\get_package_data(), Packages\check_requirements(), and version_compare(). Each sub-call can fail independently. Testing the full matrix (8 combinations) requires either:

• Reflection + partial mocking (brittle)
• Refactoring to inject Package objects that return controlled values

Suggested fix: Extract the per-package logic into a testable method or make it protected static so a test subclass can call it.

5.2 verify_signature_on_download() — Tight coupling to WP_Upgrader

The function receives $upgrader by type-hint, calls $upgrader->download_package(), then verify_file_signature(). Mocking a WP_Upgrader is impractical because download_package() is final or does real filesystem work.

Suggested fix: Extract the download-and-verify step into a separate function:

function download_and_verify( string $url, string $expected_signature, array $trusted_keys ): string|WP_Error

Then test the wrapper with a mock HTTP layer, and leave the hook glue in verify_signature_on_download untested (integration-tested instead).

5.3 upgrader_source_selection() — Filesystem operations

Renames directories on disk. Testing requires real temp directories, which is doable in PHPUnit but cumbersome.

Suggested fix: Extract the path-munging logic (hash detection, destination computation) from the filesystem operations:

function compute_destination_path( string $source, string $remote_source, array $hook_extra ): string

Unit-test the computation; leave the rename() call for integration tests.

---

6. Already-Good Tests Worth Noting

These tests are solid and should serve as patterns for new tests:

• SignatureVerificationTest.php — Comprehensive coverage of the crypto pipeline. Tests key decoding, key matching, valid/tampered/wrong-key/wrong-signature verification. The fixture generation is clean. Good use of @group signature.

• MetadataDocumentFromDataTest.php — Tests all mandatory field validation, optional field defaults, multiple releases, and error propagation. The factory pattern (MetadataDocumentFactory) keeps test data clean.

• DisplayPluginUpdateErrorTest.php — Good output buffering approach for testing HTML generation. Tests no-output, error-output, active-class, XSS sanitization, and colspan. Strong model for other rendering tests.

• PickArtifactByLangTest.php — Exhaustive locale matching: exact, prefix, fallback, underscore normalization. The test_filter_can_override_selection test validates the extension point.

• direct-install.spec.ts (browser) — Accessibility-first testing. Verifies labels, ARIA attributes, keyboard navigation, and heading hierarchy before testing functionality. This is the right priority order for UI tests.

---

7. Prioritized Action Items

Immediate (✅ done)

#	Status	Action	Effort
1	✅	Delete SampleTest.php	Trivial
2	✅	Remove redundant assertions from GetPackagesTest	Trivial
3	✅	Remove duplicate assertions from AvatarHttpTest	Trivial
4	✅	Remove WordPress core filter-fire test	Trivial
5	✅	Remove bootstrap filter-registration test	Trivial
6	✅	Replace reflection with Updater::reset()	Small

High Priority (✅ done)

#	Status	Action	Effort
7	✅	upgrader_source_selection unit tests	Medium
8	✅	verify_signature_on_download unit tests	Medium
9	✅	get_trusted_keys base64 recoding unit test	Small
10	✅	Package::get_release() memoization unit tests	Small

Zero-Refactoring (✅ ALL DONE)

#	Status	Action	Maps to	Commit
11	✅	Fix transient internals assertion	2.4	c2eeb9e
12	✅	Replace fixture-structure assertions with behavioral ones	2.3	c2eeb9e
13	✅	Multi-key trust test (two fair keys documented as intentional)	3.2	c2eeb9e
14	✅	Move pipeline-mock tests from unit to integration layer	2.2	a180991
15	✅	Test plugin_api_details with mocked DID pipeline	4.2	2b675f2
16	✅	Error propagation e2e (error → transient skip → error row)	4.5	2b675f2
17	✅	Beef up browser test assertions (error row seeding)	4.6	2b675f2
18	✅	Theme update HTML unit tests	4.4	27ef1fb

Needs Refactoring (⏳ blocked)

All 5 remaining items require production code changes — none are doable without refactoring.

#	Status	Action	Why blocked
19	⏳	update_site_transient unit tests	private static — needs to become protected
20	⏳	Extract per-package logic from update_site_transient()	Same function, same blocker
21	⏳	Refactor verify_signature_on_download into testable + glue layers	Cleanup only; function already tested (3.1 ✅)
22	⏳	Refactor upgrader_source_selection path computation into pure function	Cleanup only; function already tested (3.5 ✅)
23	⏳	Make update_site_transient protected instead of private	Enables #19, #20