test: add comprehensive test suite (~1,266 tests)#1172
Conversation
WalkthroughAdds extensive JS/TS and PHP unit tests, two test-documents cataloging bugs and a testing roadmap, and test-tooling adjustments (PHPUnit downgrade + polyfills, Jest mapping). No production API or runtime code is changed; the diff is test/docs/config-only. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/php/tests/RestAPI/Test_Webhook_REST_Endpoints.php (1)
64-74:⚠️ Potential issue | 🟡 Minor
$_SERVERmodifications not cleaned up after test.Lines 66-67 set
$_SERVER['HTTP_USER_AGENT']and$_SERVER['HTTP_AUTHORIZATION']but these are never restored. This could leak state into subsequent tests. Consider saving/restoring the original values, or unsetting them intearDown.Proposed fix
+ /** + * Original SERVER values to restore. + */ + private $original_server = []; + public function tearDown(): void { global $wp_rest_server; $wp_rest_server = null; + + // Restore original $_SERVER values. + foreach ( $this->original_server as $key => $value ) { + if ( null === $value ) { + unset( $_SERVER[ $key ] ); + } else { + $_SERVER[ $key ] = $value; + } + } + $this->original_server = []; parent::tearDown(); }And in the test method:
public function test_webhook_verify_endpoint_security() { + $this->original_server['HTTP_USER_AGENT'] = $_SERVER['HTTP_USER_AGENT'] ?? null; + $this->original_server['HTTP_AUTHORIZATION'] = $_SERVER['HTTP_AUTHORIZATION'] ?? null; + $_SERVER['HTTP_USER_AGENT'] = 'PopupMakerUpgrader'; $_SERVER['HTTP_AUTHORIZATION'] = 'Bearer test-token';
🤖 Fix all issues with AI agents
In `@claudedocs/BUGS-FOUND-BY-TESTS.md`:
- Around line 59-72: Add a short note to Bug `#4`'s fix section stating that the
same empty-title validation bug appears in CTA validation at
packages/core-data/src/call-to-actions/validation.ts where the condition
callToAction.title && !callToAction.title?.length wrongly treats empty strings
as present; reference that symbol and suggest the same remediation (change the
conditional to properly detect empty titles or normalize/trim title before
checking) so maintainers know to apply the fix to both popup and CTA validation.
In `@packages/core-data/src/call-to-actions/__tests__/validation.test.ts`:
- Around line 47-57: The test name is misleading: update the test description
for the case that asserts validateCallToAction returns true for an empty-string
title; locate the test that calls validateCallToAction (the it( 'returns error
for CTA with empty string title', ...) block) and change the string to
accurately reflect the behavior (e.g., "returns true for CTA with empty string
title" or "treats empty string title as valid") so the name matches the
assertion and inline comment.
In `@packages/core-data/src/popups/__tests__/reducer.test.ts`:
- Around line 534-552: The test for ACTION_TYPES.INVALIDATE_RESOLUTION is
ineffective because resolutionState.getPopup is initialized as a flat object
without an ID key; update the test's existing state so resolutionState.getPopup
contains an ID-keyed entry (e.g., getPopup: { 1: { status:
DispatchStatus.Success } }) before calling reducer(reducer, { type:
ACTION_TYPES.INVALIDATE_RESOLUTION, payload: { id: 1, operation: 'getPopup' }
}), then assert that (state.resolutionState.getPopup as any)[1] is undefined to
verify the reducer actually removed the ID entry; reference symbols: reducer,
ACTION_TYPES.INVALIDATE_RESOLUTION, initialState, State,
resolutionState.getPopup, DispatchStatus.Success.
In `@packages/core-data/src/settings/__tests__/reducer.test.ts`:
- Around line 262-282: The test for INVALIDATE_RESOLUTION is asserting a key
that was never set, so update the test setup to create a keyed resolution before
dispatching: modify the existing state’s resolutionState.fetchSettings to
include a numeric key (e.g., 1: { status: DispatchStatus.Success }) so the
reducer(INVALIDATE_RESOLUTION) actually removes that entry; then assert that
(state.resolutionState.fetchSettings as any)[1] is undefined (or alternatively
assert that resolutionState.fetchSettings is cleared) — locate this in the test
around reducer, ACTION_TYPES.INVALIDATE_RESOLUTION, and
resolutionState.fetchSettings.
In `@tests/php/tests/PUM_Admin_Settings_Test.php`:
- Around line 397-404: The test test_sanitize_objects_invalid_json currently
only checks the key exists; change it to assert the exact value and type
returned by PUM_Admin_Settings::sanitize_objects for invalid JSON (i.e., replace
the assertArrayHasKey with an assertion like assertSame($expected,
$result['key1']) where $expected is the actual output produced by
sanitize_objects for the 'not valid json at all' input) so the test pins the
concrete behavior rather than passing vacuously.
In `@tests/php/tests/PUM_ConditionCallbacks_Test.php`:
- Around line 340-360: The test test_post_type_index_modifier_on_archive
registers a custom post type 'pum_test_cpt' via register_post_type but never
unregisters it, leaking into global $wp_post_types; add a matching
unregister_post_type('pum_test_cpt') to clean up (either at the end of
test_post_type_index_modifier_on_archive or in the test class tearDown) so the
registration from register_post_type('pum_test_cpt', ...) is removed after the
test runs.
In `@tests/php/tests/PUM_DB_Subscribers_Test.php`:
- Around line 630-631: Remove the unused variable assignment or assert its
value: either drop "$id1 =" and just call $this->db->insert(...) if the inserted
ID isn't needed, or keep "$id1 = $this->db->insert(...)" and add an assertion
that uses $id1 (for example asserting it's an integer > 0 or equals the ID
returned by a subsequent query) so the test uses the returned ID from
PUM_DB_Subscribers_Test's call to $this->db->insert.
- Around line 390-395: The test names test_update_negative_id_returns_false and
test_delete_negative_id_returns_false are misleading because the assertions
expect true (due to absint(-5) -> 5); rename the methods to
test_update_negative_id_returns_true and test_delete_negative_id_returns_true
(or alternatively change assertTrue to assertFalse if you intend to test a false
result), and update any related docblock/comments to match; locate the test
methods by their names (test_update_negative_id_returns_false,
test_delete_negative_id_returns_false) and update the method name and any
references accordingly to keep behavior and names consistent with absint()
semantics.
🧹 Nitpick comments (34)
tests/php/tests/PUM_Cookies_Test.php (2)
314-322: Unused$idvariable in foreach loop.The static analysis tool flagged
$idas unused on line 319. Since you're only validating the values, use$_or just omit the key.🔧 Suggested fix
- foreach ( $list as $id => $name ) { + foreach ( $list as $name ) { $this->assertIsString( $name, 'Cookie name should be a string.' ); }
360-411: Filter cleanup won't run if an assertion fails mid-test.If any assertion before
remove_all_filters()throws, the filter persists into subsequent tests. This is unlikely to cause issues here sincesetUpresets the singleton, but for robustness you could move filter removal totearDownor accept the minor risk.tests/php/tests/PUM_DB_Subscribers_Test.php (2)
671-674: Duplicate oftest_table_name_includes_prefix(line 31).
test_table_name_methodis identical in logic and assertion totest_table_name_includes_prefix. Consider removing one.
23-26: Consider callingcreate_table()once insetUp()to reduce repetition.Nearly every test method begins with
$this->db->create_table(). Moving it intosetUp()(which already instantiates the DB object) would eliminate ~30 duplicate calls. The dedicatedtest_create_table_*tests can still call it explicitly.Suggested change
public function setUp(): void { parent::setUp(); $this->db = new PUM_DB_Subscribers(); + $this->db->create_table(); }packages/utils/src/lib/__tests__/debug.test.ts (3)
8-15: Dead test infrastructure andanytype usage.
storageListenersis populated but never read or invoked in any test — it's dead code along with theorigAddEventListenerreference (also never used to restore or delegate).- The
handlerparameter usesany, which violates the TypeScript coding guideline.- Because
origAddEventListeneris never called as a fallback, all non-'storage'event registrations during module loading are silently dropped, which could mask issues.Consider either removing this infrastructure entirely (if storage-event testing isn't needed yet) or wiring it up properly:
Proposed cleanup
-// Mock window.addEventListener to capture the storage listener. -const storageListeners: Array< () => void > = []; -const origAddEventListener = window.addEventListener; -window.addEventListener = jest.fn( ( event: string, handler: any ) => { - if ( event === 'storage' ) { - storageListeners.push( handler ); - } -} ); +// Mock window.addEventListener to capture the storage listener while +// preserving default behavior for other events. +const storageListeners: Array< ( e: StorageEvent ) => void > = []; +const origAddEventListener = window.addEventListener.bind( window ); +window.addEventListener = jest.fn( + ( event: string, handler: EventListenerOrEventListenerObject ) => { + if ( event === 'storage' ) { + storageListeners.push( + handler as ( e: StorageEvent ) => void + ); + } else { + origAddEventListener( event, handler ); + } + } +) as typeof window.addEventListener;As per coding guidelines: "Do not use
anytype in TypeScript; useunknownfor dynamic values outside of the type system, or find/define the type."
111-137: Consider adding a call-count assertion before inspectingmock.calls.The selector, action, resolver, and state tests jump straight to checking
mock.calls[0][0]without first assertingtoHaveBeenCalledTimes(1)(as done in the component and effect tests above). IfgroupCollapsedisn't called,mock.calls[0]isundefinedand the test fails with a confusingTypeErrorinstead of a clear assertion message.Example for one test (apply similarly to others)
it( 'logs selector debug messages', () => { debugModule.debug.selector( 'getPopups' ); + expect( consoleSpy.groupCollapsed ).toHaveBeenCalledTimes( 1 ); expect( consoleSpy.groupCollapsed.mock.calls[ 0 ][ 0 ] ).toMatch( /Select:getPopups/ ); } );
192-197: Comment/regex mismatch on timestamp format.The comment on line 195 says
HH:MM:SS.mmmZbut the regex only validates[HH:MM:SS— it doesn't check milliseconds or the trailingZ. If the format assertion is intentionally loose, consider updating the comment; if milliseconds matter, tighten the regex.tests/php/tests/Logging_Service_Test.php (2)
17-42: Consider extracting repeated setup intosetUp()to reduce boilerplate.Every test method repeats
create_mock_container()→create_logging_instance()→set_private_property(... 'content' ...)→set_private_property(... 'is_writable' ...). Promoting$service(and optionally$container) to instance properties initialized insetUp()would cut significant duplication.♻️ Suggested refactor
class Logging_Service_Test extends WP_UnitTestCase { + + /** + * `@var` Logging + */ + private $service; + + /** + * Set up test fixtures. + */ + public function setUp(): void { + parent::setUp(); + $container = $this->create_mock_container(); + $this->service = $this->create_logging_instance( $container ); + } + /** * Test disabled returns true when PUM_DISABLE_LOGGING is defined. */ public function test_disabled_returns_true_with_constant() { - $container = $this->create_mock_container(); - $service = $this->create_logging_instance( $container ); - // Without the constant defined, disabled() should return false. - $this->assertFalse( $service->disabled(), 'Should not be disabled without the constant.' ); + $this->assertFalse( $this->service->disabled(), 'Should not be disabled without the constant.' ); }Then each test would use
$this->servicedirectly, only callingset_private_propertyfor the specific state it needs.
243-262: Coding guidelines recommend Mockery for PHP mocking.The anonymous class works fine here, but the project guidelines specify using Mockery for test mocks. This is a low-priority alignment item since the anonymous class is simple and readable.
As per coding guidelines: "Write PHP unit tests using PHPUnit in the
tests/php/tests/directory and use Mockery for PHP mocking".tests/php/tests/PUM_Admin_Settings_Test.php (2)
116-125: UsemarkTestSkippedinstead ofassertTrue(true)for conditional branches.The
assertTrue(true)pattern appears here and in several other tests (lines 164, 212, 243, 273, 289) as a workaround for PHPUnit's "risky test" warning. This silently passes and hides the fact that nothing was actually tested.markTestSkipped()is more appropriate — it signals clearly in test output that the assertion path wasn't exercised, making it easier to notice when expected fields disappear.Example fix (apply same pattern to all occurrences)
if ( isset( $defaults['body_padding_override'] ) ) { $this->assertEquals( '15px', $defaults['body_padding_override'], 'body_padding_override default should be 15px.' ); } else { - // If the field doesn't exist, add an assertion to prevent risky test warning. - $this->assertTrue( true, 'Field body_padding_override not found in defaults.' ); + $this->markTestSkipped( 'Field body_padding_override not found in defaults.' ); }
219-275: Duplicated license field lookup logic across two tests.Both
test_sanitize_settings_license_key_star_mask_preservedandtest_sanitize_settings_license_key_new_valuecontain identical loops to find the firstlicense_keyfield. Consider extracting a small private helper (e.g.,find_first_field_by_type) to reduce duplication.tests/php/tests/PUM_Models_CallToAction_Test.php (1)
229-236: Consider asserting the actual default description text.The relevant code snippet from
CallToAction.php(line 206) shows the default is'This content is restricted.'. This test only checks non-empty + string, making it unable to catch regressions in the default message. The comment on line 234 already knows the expected value.Suggested improvement
public function test_get_description_returns_default_message() { $cta = $this->create_cta(); $desc = $cta->get_description(); - $this->assertNotEmpty( $desc ); - // Default message is "This content is restricted." when get_the_excerpt() is empty. - $this->assertIsString( $desc ); + $this->assertSame( 'This content is restricted.', $desc ); }tests/php/tests/RestAPI/Test_License_REST_Endpoints.php (1)
60-101: Dead code aftermarkTestSkipped— consider documenting intent more clearly.All code after
markTestSkipped()on line 64 is unreachable. While the tests are well-structured for future use, note that lines 85-90 define a globalplugin()function which could collide with the realplugin()function or other test classes if the skip is ever removed. Consider using a namespace or a different mocking strategy when re-enabling.tests/php/phpunit.xml (2)
2-9:convertErrorsToExceptions,convertNoticesToExceptions,convertWarningsToExceptionsare deprecated in PHPUnit 9.5+.These attributes were removed in PHPUnit 10 and generate deprecation notices in 9.5+. They can be safely removed since they default to
truein PHPUnit 9.
15-25:<filter><whitelist>generates deprecation warnings in PHPUnit 9.6 — migrate to<coverage>syntax.The old XML schema is deprecated (PHPUnit 9.5+). While it still works, it emits deprecation warnings in 9.6. Replace it with the modern
<coverage>syntax using<include>and<exclude>elements:Proposed migration to `` syntax
- <filter> - <whitelist processUncoveredFilesFromWhitelist="false"> - <directory suffix=".php">../../classes/</directory> - <directory suffix=".php">../../includes/</directory> - <exclude> - <directory suffix=".php">../../includes/legacy/</directory> - <directory suffix=".php">../../vendor/</directory> - <directory suffix=".php">../../vendor-prefixed/</directory> - </exclude> - </whitelist> - </filter> + <coverage processUncoveredFiles="false"> + <include> + <directory suffix=".php">../../classes/</directory> + <directory suffix=".php">../../includes/</directory> + </include> + <exclude> + <directory suffix=".php">../../includes/legacy/</directory> + <directory suffix=".php">../../vendor/</directory> + <directory suffix=".php">../../vendor-prefixed/</directory> + </exclude> + </coverage>packages/components/src/lib/url-control/__tests__/is-url-like.test.ts (2)
1-10: Mock of@wordpress/urlmay diverge from the realisURLbehavior.The native
URLconstructor accepts schemes likeftp:andmailto:that WordPress'sisURLmay reject. If the realisURLhas stricter validation, edge-case tests (lines 78–84) could pass here but fail in production. Consider verifying the realisURLbehavior for these protocols, or add a comment noting the approximation.
12-90: Good coverage of URL-like detection scenarios.Tests cover full URLs, www-prefixed strings, hash links, non-URL strings, and edge cases. The use of
toBeTruthy()/toBeFalsy()vstoBe(true)/toBe(false)is intentionally inconsistent — consider using strict boolean checks throughout for consistency, sinceisURLLikereturnsboolean.claudedocs/testing-gap-analysis.md (1)
195-206: Add language specifiers to fenced code blocks.The fenced code blocks at lines 195, 213, and 231 lack a language identifier, which triggers MD040 warnings and prevents syntax highlighting. Adding a language (e.g.,
```textor```plaintext) would resolve this.packages/core-data/src/call-to-actions/__tests__/reducer.test.ts (1)
25-41: Pervasive use ofas anyin mock helpers.The
mockCtaandmockEditablehelpers cast return values toany, bypassing type safety. While pragmatic for tests, the coding guidelines prohibit theanytype. Consider defining minimal type-compatible objects or usingPartial<CallToAction<'edit'>>to preserve some type checking.That said, this is a low-priority concern in test code and can be deferred. As per coding guidelines: "Do not use
anytype in TypeScript; useunknownfor dynamic values outside of the type system, or find/define the type."claudedocs/BUGS-FOUND-BY-TESTS.md (1)
207-210: Bug#12is explicitly a duplicate of Bug#6— consider removing.This entry adds no new information beyond "confirmed by Batch 2." Consolidating it into Bug
#6as a note would keep the document cleaner.tests/php/tests/PUM_ConditionCallbacks_Test.php (1)
990-1004: Custom taxonomygenreregistered in multiple tests without cleanup.
register_taxonomy('genre', ...)is called in five separate test methods. While WP_UnitTestCase handles DB rollback, the taxonomy registration persists in the global$wp_taxonomies. Consider registering it once insetUp()or adding cleanup in atearDown()method to avoid inter-test leakage.The same applies to the
colortaxonomy registered in lines 1288-1361.Also applies to: 1009-1021, 1030-1046, 1051-1067, 1072-1088
packages/registry/src/__tests__/index.test.ts (1)
199-251: Subscriber tests are tightly coupled touseSyncExternalStoremock internals.The tests at lines 200-251 extract the
subscribecallback by overriding theuseSyncExternalStoremock from inside the test and callingregistry.useItems()to trigger the subscription. This works but is fragile — any refactor of how the registry manages subscriptions (e.g., exposingsubscribedirectly) would break these tests without changing behavior.This is acceptable for now since the registry's public API doesn't directly expose subscribe/unsubscribe, but worth noting for future maintainability.
tests/php/tests/PUM_Utils_Options_Test.php (1)
424-433: Unused$resultvariable — either assert on it or drop the assignment.PHPMD correctly flags
$resultas unused. Since the test verifies that existing keys survive a delete of a non-existent key, you likely want to assert the return value too.Proposed fix
- $result = PUM_Utils_Options::delete( 'nonexistent' ); + PUM_Utils_Options::delete( 'nonexistent' );Or, if you want to document the expected return:
- $result = PUM_Utils_Options::delete( 'nonexistent' ); - - $all = get_option( 'popmake_settings' ); + $result = PUM_Utils_Options::delete( 'nonexistent' ); + // update_option returns false when the value is unchanged. + $this->assertFalse( $result ); + $all = get_option( 'popmake_settings' );packages/core-data/src/call-to-actions/__tests__/selectors.test.ts (1)
50-65:as anycasts on mock helpers violate the TypeScript coding guideline.The project guideline states: "Do not use
anytype in TypeScript." Consider usingPartial<CallToAction<'edit'>>(or the appropriate type) with anascast to the full type, or define a lightweight test-only type. This is a low-priority nit for test code.Example
-const mockCta = ( id: number, title = `CTA ${ id }` ) => - ( { - id, - title, - status: 'publish', - settings: { type: 'link', url: '' }, - } ) as any; +const mockCta = ( id: number, title = `CTA ${ id }` ) => + ( { + id, + title, + status: 'publish', + settings: { type: 'link', url: '' }, + } ) as unknown as CallToAction< 'edit' >;As per coding guidelines, "Do not use
anytype in TypeScript; useunknownfor dynamic values outside of the type system, or find/define the type."tests/php/tests/LinkClickTracking_Test.php (1)
43-48: UsingstdClassas a container may be fragile if theServicebase class evolves.Currently works, but if the
Serviceconstructor orLinkClickTracking::init()later accesses$container->get(...),stdClasswill cause a fatal error. Consider using a minimal anonymous class with aget()stub (similar to the pattern inPUM_Services_Options_Test), which would be more resilient to future changes.tests/php/tests/PUM_Triggers_Test.php (1)
303-311: Consider using$idin the assertion message for better test diagnostics.PHPMD flags
$idas unused. While the foreach structure implicitly validates keys, using$idin the assertion message would both silence the warning and improve failure diagnostics.Proposed fix
foreach ( $list as $id => $name ) { - $this->assertIsString( $name, 'Trigger name should be a string.' ); + $this->assertIsString( $name, "Trigger '$id' name should be a string." ); }tests/php/tests/PUM_Conditions_Test.php (1)
362-369: Same$idunused variable pattern — use it in the assertion message.Same as the
PUM_Triggers_Testfeedback: using$idin the assertion message improves diagnostics and silences the PHPMD warning.Proposed fix
foreach ( $conditions as $id => $name ) { - $this->assertIsString( $name, 'Condition name should be a string.' ); + $this->assertIsString( $name, "Condition '$id' name should be a string." ); }tests/php/tests/PUM_Utils_Fields_Test.php (1)
709-717: Minor: suppress or remove the unused$outputvariable.
$outputis captured but never asserted on. You can callob_get_clean()without assignment to silence the PHPMD warning.♻️ Suggested cleanup
ob_start(); PUM_Utils_Fields::render_field( [ 'type' => 'nonexistent_type_xyz' ] ); - $output = ob_get_clean(); + ob_get_clean();tests/php/tests/PUM_Utils_ArrayTest.php (1)
1148-1153: Simplify the convoluted assertion on Line 1149.The
<=> 0 ? -1 : 1expression obscures the intent. The next two lines (1150–1151) already assert the same behavior more clearly. Consider removing or simplifying Line 1149.♻️ Suggested simplification
public function test_sort_by_sort() { - $this->assertSame( -1, PUM_Utils_Array::sort_by_sort( [ 'sort' => 1 ], [ 'sort' => 2 ] ) <=> 0 ? -1 : 1 ); $this->assertLessThan( 0, PUM_Utils_Array::sort_by_sort( [ 'sort' => 1 ], [ 'sort' => 10 ] ) ); $this->assertGreaterThan( 0, PUM_Utils_Array::sort_by_sort( [ 'sort' => 10 ], [ 'sort' => 1 ] ) ); $this->assertSame( 0, PUM_Utils_Array::sort_by_sort( [ 'sort' => 5 ], [ 'sort' => 5 ] ) ); }packages/core-data/src/popups/__tests__/selectors.test.ts (1)
18-30:applyPatchmock only handles single-segment JSON Pointer paths.
patch.path.replace('/', '')removes only the first/, so a path like/settings/displaywould produce the keysettings/displayinstead of nesting. This is fine for the current tests (all use top-level paths like/title), but worth noting if deeper-path tests are added later.tests/php/tests/REST_Connect_Test.php (2)
48-60: Extract repeatedcreatePartialMock+get_install_webhook_argsto a fixture.The pattern
$this->createPartialMock(Connect::class, [])followed by$controller->get_install_webhook_args()is repeated in nearly every test. Moving the controller and args intosetUpproperties (e.g.,$this->controllerand$this->install_args) would cut ~60 lines of boilerplate.♻️ Proposed refactor
+ /** + * Controller instance. + * + * `@var` Connect + */ + private $controller; + + /** + * Install webhook args. + * + * `@var` array + */ + private $install_args; + public function setUp(): void { parent::setUp(); do_action( 'rest_api_init' ); + $this->controller = $this->createPartialMock( Connect::class, [] ); + $this->install_args = $this->controller->get_install_webhook_args(); }Then replace all instances of:
$controller = $this->createPartialMock( Connect::class, [] ); $args = $controller->get_install_webhook_args();with references to
$this->controllerand$this->install_args.
496-516: Mock service setup is also repeated — consider a helper method.The
getMockBuilder(stdClass::class)->addMethods([...])->getMock()+ reflection pattern for settingconnect_serviceis repeated in 6 tests (Lines 496, 521, 542, 632, 652, 672). A private helper likecreateControllerWithMockService()would simplify these.tests/php/tests/PUM_Analytics_Expanded_Test.php (2)
94-114: Unused closure parameters can be simplified.The PHPMD hint about unused
$popup_idand$argsat Line 99 is valid — since neither is used, the closure can be simplified.♻️ Proposed fix
add_action( 'pum_analytics_open', - function ( $popup_id, $args ) use ( &$fired ) { + function () use ( &$fired ) { $fired = true; }, 10, 2 );
396-411: Weak assertion intest_analytics_enabled_disabled_by_option.The test sets
disable_analyticstotruebut only assertsassertIsBool(Line 410) instead ofassertFalse. This makes the test nearly a no-op sinceanalytics_enabled()always returns a boolean. Consider investigating howpum_get_optionreads the setting so this can assertassertFalsefor the expected behavior.
| ## 3. `omit.ts`: Implementation Picks Instead of Omitting | ||
|
|
||
| **Severity**: High | ||
| **Location**: `packages/utils/src/lib/omit.ts` | ||
| **Test**: `packages/utils/src/lib/__tests__/omit.test.ts` | ||
|
|
||
| **Problem**: The function is typed as `Omit<T, K>` but the implementation copies the specified keys TO the result instead of excluding them FROM the result. It behaves like `pick()`, not `omit()`. | ||
|
|
||
| **Impact**: Any code calling `omit(obj, ['a', 'b'])` expecting `a` and `b` to be removed will instead get ONLY `a` and `b`. If callers adapted to the buggy behavior, fixing this will break them. | ||
|
|
||
| **Fix**: Either: | ||
| 1. Fix the implementation to actually omit keys, OR | ||
| 2. Rename it to `pick` if that's the intended behavior and update the type signature | ||
|
|
There was a problem hiding this comment.
Bug #3 (omit.ts): the title validation logic in the fix section of Bug #4 also applies to CTA validation.
Bug #4 documents the empty-title issue for popups, but the same pattern exists in packages/core-data/src/call-to-actions/validation.ts (line 29: callToAction.title && !callToAction.title?.length). Consider adding a note that Bug #4 also affects CTA validation.
🤖 Prompt for AI Agents
In `@claudedocs/BUGS-FOUND-BY-TESTS.md` around lines 59 - 72, Add a short note to
Bug `#4`'s fix section stating that the same empty-title validation bug appears in
CTA validation at packages/core-data/src/call-to-actions/validation.ts where the
condition callToAction.title && !callToAction.title?.length wrongly treats empty
strings as present; reference that symbol and suggest the same remediation
(change the conditional to properly detect empty titles or normalize/trim title
before checking) so maintainers know to apply the fix to both popup and CTA
validation.
| it( 'returns error for CTA with empty string title', () => { | ||
| // `callToAction.title` = '' is falsy, so `callToAction.title && !callToAction.title?.length` | ||
| // evaluates to '' (falsy) → condition is false → returns true. | ||
| // This is the actual behavior of the code. | ||
| const result = validateCallToAction( { | ||
| id: 1, | ||
| title: '', | ||
| } as any ); | ||
|
|
||
| expect( result ).toBe( true ); | ||
| } ); |
There was a problem hiding this comment.
Misleading test name: says "returns error" but asserts true.
The test is named 'returns error for CTA with empty string title' but the assertion on line 56 expects result to be true (i.e., valid). The inline comment explains the behavior correctly, but the test name is confusing and could mislead future developers.
Suggested name fix
- it( 'returns error for CTA with empty string title', () => {
+ it( 'BUG: does NOT catch empty string title (returns true)', () => {🤖 Prompt for AI Agents
In `@packages/core-data/src/call-to-actions/__tests__/validation.test.ts` around
lines 47 - 57, The test name is misleading: update the test description for the
case that asserts validateCallToAction returns true for an empty-string title;
locate the test that calls validateCallToAction (the it( 'returns error for CTA
with empty string title', ...) block) and change the string to accurately
reflect the behavior (e.g., "returns true for CTA with empty string title" or
"treats empty string title as valid") so the name matches the assertion and
inline comment.
| public function test_sanitize_objects_invalid_json() { | ||
| $input = [ | ||
| 'key1' => 'not valid json at all', | ||
| ]; | ||
| $result = PUM_Admin_Settings::sanitize_objects( $input ); | ||
| // json_decode returns null for invalid json, then object_to_array handles it. | ||
| $this->assertArrayHasKey( 'key1', $result ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Weak assertion — test passes regardless of behavior.
This test only asserts the key still exists, but doesn't verify what value sanitize_objects produces for invalid JSON input. If the implementation changes to silently drop the key or return an unexpected type, this test would still pass.
Proposed fix
public function test_sanitize_objects_invalid_json() {
$input = [
'key1' => 'not valid json at all',
];
$result = PUM_Admin_Settings::sanitize_objects( $input );
- // json_decode returns null for invalid json, then object_to_array handles it.
- $this->assertArrayHasKey( 'key1', $result );
+ // json_decode returns null for invalid JSON; document the actual expected behavior.
+ $this->assertArrayHasKey( 'key1', $result, 'Key should still be present after sanitization.' );
+ $this->assertNull( $result['key1'], 'Invalid JSON should result in null after decode.' );
}Adjust the expected value to match whatever sanitize_objects actually returns for invalid JSON — the point is to pin the behavior explicitly.
🤖 Prompt for AI Agents
In `@tests/php/tests/PUM_Admin_Settings_Test.php` around lines 397 - 404, The test
test_sanitize_objects_invalid_json currently only checks the key exists; change
it to assert the exact value and type returned by
PUM_Admin_Settings::sanitize_objects for invalid JSON (i.e., replace the
assertArrayHasKey with an assertion like assertSame($expected, $result['key1'])
where $expected is the actual output produced by sanitize_objects for the 'not
valid json at all' input) so the test pins the concrete behavior rather than
passing vacuously.
| public function test_post_type_index_modifier_on_archive() { | ||
| // 'post' type doesn't have a post type archive (uses blog page instead). | ||
| // Use a custom post type that has has_archive = true. | ||
| register_post_type( 'pum_test_cpt', [ | ||
| 'public' => true, | ||
| 'has_archive' => true, | ||
| ] ); | ||
|
|
||
| $this->factory->post->create( [ 'post_type' => 'pum_test_cpt', 'post_status' => 'publish' ] ); | ||
|
|
||
| $this->go_to( get_post_type_archive_link( 'pum_test_cpt' ) ); | ||
|
|
||
| $condition = [ | ||
| 'target' => 'pum_test_cpt_index', | ||
| 'settings' => [], | ||
| ]; | ||
|
|
||
| $result = PUM_ConditionCallbacks::post_type( $condition ); | ||
|
|
||
| $this->assertTrue( $result, 'post_index should match on a post type archive page.' ); | ||
| } |
There was a problem hiding this comment.
Missing unregister_post_type() for pum_test_cpt.
The custom post type registered at line 343 is never unregistered, unlike my_cpt (line 758) and book (line 1431). While WP_UnitTestCase rolls back the database, the post type registration persists in the global $wp_post_types array and could leak into subsequent tests.
Suggested fix
$this->assertTrue( $result, 'post_index should match on a post type archive page.' );
+
+ unregister_post_type( 'pum_test_cpt' );
}🤖 Prompt for AI Agents
In `@tests/php/tests/PUM_ConditionCallbacks_Test.php` around lines 340 - 360, The
test test_post_type_index_modifier_on_archive registers a custom post type
'pum_test_cpt' via register_post_type but never unregisters it, leaking into
global $wp_post_types; add a matching unregister_post_type('pum_test_cpt') to
clean up (either at the end of test_post_type_index_modifier_on_archive or in
the test class tearDown) so the registration from
register_post_type('pum_test_cpt', ...) is removed after the test runs.
| public function test_update_negative_id_returns_false() { | ||
| $this->db->create_table(); | ||
| $result = $this->db->update( -5, [ 'name' => 'Nope' ] ); | ||
| // absint(-5) = 5, which is a valid positive ID, so update proceeds. | ||
| $this->assertTrue( $result ); | ||
| } |
There was a problem hiding this comment.
Misleading test method name — asserts true but name says returns_false.
The method test_update_negative_id_returns_false actually asserts assertTrue. The docblock and comments correctly describe the absint() behavior, but the method name contradicts the actual assertion. Same issue applies to test_delete_negative_id_returns_false on line 472.
Suggested rename
- public function test_update_negative_id_returns_false() {
+ public function test_update_negative_id_returns_true_due_to_absint() {And on line 472:
- public function test_delete_negative_id_returns_false() {
+ public function test_delete_negative_id_returns_true_due_to_absint() {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function test_update_negative_id_returns_false() { | |
| $this->db->create_table(); | |
| $result = $this->db->update( -5, [ 'name' => 'Nope' ] ); | |
| // absint(-5) = 5, which is a valid positive ID, so update proceeds. | |
| $this->assertTrue( $result ); | |
| } | |
| public function test_update_negative_id_returns_true_due_to_absint() { | |
| $this->db->create_table(); | |
| $result = $this->db->update( -5, [ 'name' => 'Nope' ] ); | |
| // absint(-5) = 5, which is a valid positive ID, so update proceeds. | |
| $this->assertTrue( $result ); | |
| } |
🤖 Prompt for AI Agents
In `@tests/php/tests/PUM_DB_Subscribers_Test.php` around lines 390 - 395, The test
names test_update_negative_id_returns_false and
test_delete_negative_id_returns_false are misleading because the assertions
expect true (due to absint(-5) -> 5); rename the methods to
test_update_negative_id_returns_true and test_delete_negative_id_returns_true
(or alternatively change assertTrue to assertFalse if you intend to test a false
result), and update any related docblock/comments to match; locate the test
methods by their names (test_update_negative_id_returns_false,
test_delete_negative_id_returns_false) and update the method name and any
references accordingly to keep behavior and names consistent with absint()
semantics.
| $id1 = $this->db->insert( [ 'email' => 'num1@example.com', 'popup_id' => 42 ] ); | ||
| $this->db->insert( [ 'email' => 'num2@example.com', 'popup_id' => 99 ] ); |
There was a problem hiding this comment.
Unused variable $id1.
$id1 is assigned but never referenced. Either remove the assignment or use it in the assertion to make the test more precise (e.g., verifying the matched row's ID).
Suggested fix
- $id1 = $this->db->insert( [ 'email' => 'num1@example.com', 'popup_id' => 42 ] );
+ $this->db->insert( [ 'email' => 'num1@example.com', 'popup_id' => 42 ] );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $id1 = $this->db->insert( [ 'email' => 'num1@example.com', 'popup_id' => 42 ] ); | |
| $this->db->insert( [ 'email' => 'num2@example.com', 'popup_id' => 99 ] ); | |
| $this->db->insert( [ 'email' => 'num1@example.com', 'popup_id' => 42 ] ); | |
| $this->db->insert( [ 'email' => 'num2@example.com', 'popup_id' => 99 ] ); |
🧰 Tools
🪛 PHPMD (2.15.0)
[warning] 630-630: Avoid unused local variables such as '$id1'. (undefined)
(UnusedLocalVariable)
🤖 Prompt for AI Agents
In `@tests/php/tests/PUM_DB_Subscribers_Test.php` around lines 630 - 631, Remove
the unused variable assignment or assert its value: either drop "$id1 =" and
just call $this->db->insert(...) if the inserted ID isn't needed, or keep "$id1
= $this->db->insert(...)" and add an assertion that uses $id1 (for example
asserting it's an integer > 0 or equals the ID returned by a subsequent query)
so the test uses the returned ID from PUM_DB_Subscribers_Test's call to
$this->db->insert.
- Fix cleanup in Webhook test tearDown - Replace as any with as unknown as Type across TS test files - Fix tautological INVALIDATE_RESOLUTION tests (proper per-ID state) - Modernize phpunit.xml (remove deprecated attrs, filter -> coverage) - Extract taxonomy registration to setUp/tearDown in ConditionCallbacks - DRY REST_Connect_Test with setUp fixtures and helper method - Fix analytics test using wrong option key (pum_settings -> PUM_Utils_Options API) - Strengthen weak assertions (assertIsBool -> assertFalse, assertTrue(true) -> assertIsString) - Use $id in assertion messages for better diagnostics - Remove unused variables and dead code - Fix toBeTruthy/toBeFalsy -> toBe(true)/toBe(false) for boolean returns - Consolidate Bug #12 into Bug #6, add code block language specifiers
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@claudedocs/testing-gap-analysis.md`:
- Around line 13-20: The Markdown tables in claudedocs/testing-gap-analysis.md
are missing blank lines before and after each table (MD058); edit the file to
insert a single blank line immediately above and immediately below every table
block (including the table shown and those at ranges 27-36, 37-47, 48-53) so
each table is separated by blank lines from surrounding text or other blocks,
then run markdownlint to confirm MD058 is resolved.
🧹 Nitpick comments (17)
tests/php/tests/RestAPI/Test_Webhook_REST_Endpoints.php (2)
66-76: Overly broad status code assertions may mask server errors.Accepting
500alongside expected client-error codes (e.g.,403,400) means a genuine unhandled exception in the endpoint would still pass the test. A 500 is almost never an intentional "security validation failed" response.Consider narrowing the expected sets to only the codes the endpoint should return, and treating 500 as a test failure. This applies to all similar
assertContainscalls in lines 75, 102, 120, 135, 146, 157, and 168.
194-195: Minor inconsistency: usesrest_get_server()instead of$this->server.All other tests use
$this->serverto dispatch requests, but this method callsrest_get_server()->get_routes(). They should resolve to the same global instance, so it's not a bug, but using$this->server->get_routes()would be more consistent.packages/core-data/src/settings/__tests__/selectors.test.ts (1)
141-155: Misleading test name: tests a falsy value, not an unknown key.The test is named
"returns manage_options as default for unknown permissions", but it actually sets a known key (view_block_controls) tofalseand asserts the fallback. Consider renaming to clarify it tests falsy permission values, e.g."returns manage_options when permission value is falsy". A separate test for a truly unknown key (not present in the permissions object at all) would improve coverage.tests/php/tests/PUM_Triggers_Test.php (2)
387-499: Filter cleanup can be skipped if an assertion fails, leaking filters into subsequent tests.If any assertion throws before the
remove_all_filters()call, the anonymous filter persists on the hook for the remaining test run. Move filter cleanup intotearDown()to guarantee execution regardless of test outcome.Proposed fix
public function tearDown(): void { PUM_Triggers::$instance = null; + remove_all_filters( 'pum_registered_triggers' ); + remove_all_filters( 'pum_get_triggers' ); + remove_all_filters( 'pum_trigger_cookie_fields' ); + remove_all_filters( 'pum_trigger_cookie_field' ); + remove_all_filters( 'pum_get_trigger_tabs' ); parent::tearDown(); }Then remove the inline
// Clean up.+remove_all_filters(…)calls from each test method body.
265-271: Consider guarding against a missingnamekey to avoid a PHP notice.If a trigger is registered without a
namekey,$trigger['name']would emit a notice rather than failing the assertion cleanly. AnassertArrayHasKeybeforeassertNotEmptywould give a clearer failure message, though this is minor since the production code always setsname.packages/utils/src/lib/__tests__/debug.test.ts (1)
10-10:window.addEventListeneroverride is never restored.This direct assignment isn't tracked by
jest.restoreAllMocks()(onlyjest.spyOninstances are). While Jest's jsdom sandbox prevents cross-file leakage, usingjest.spyOninstead would be cleaner and allow you to assert that the module registers itsstorageevent listener.Suggested approach
-window.addEventListener = jest.fn(); +const addEventListenerSpy = jest.spyOn( window, 'addEventListener' ).mockImplementation( () => {} );Then restore in an
afterAllor rely onjest.restoreAllMocks().tests/php/tests/PUM_ConditionCallbacks_Test.php (1)
1390-1409: Global$postleft in a dirty state after edge-case tests.
test_is_post_type_no_global_postsets$post = nullandtest_is_post_type_non_object_postsets$post = 'not_an_object'without restoring the original value.WP_UnitTestCasedoesn't auto-restore$post. This could cause order-dependent failures if tests are reordered or new tests are appended after these.Suggested fix (example for one test)
public function test_is_post_type_no_global_post() { global $post; + $original_post = $post; $post = null; $result = PUM_ConditionCallbacks::is_post_type( 'post' ); $this->assertFalse( $result, 'Should return false when global $post is null.' ); + $post = $original_post; }tests/php/tests/PUM_Conditions_Test.php (1)
536-543: Conditional assertion weakens the test — theadministratorcheck can be silently skipped.If
$rolesis unexpectedly empty, this test passes without verifying anything meaningful. In a WordPress test environment, roles are always initialized, so the guard is unnecessary. Assert non-empty unconditionally.Proposed fix
public function test_allowed_user_roles_returns_roles() { $roles = PUM_Conditions::allowed_user_roles(); $this->assertIsArray( $roles ); - // In a WP test environment, roles should include administrator. - if ( ! empty( $roles ) ) { - $this->assertArrayHasKey( 'administrator', $roles ); - } + // In a WP test environment, roles should always include administrator. + $this->assertNotEmpty( $roles, 'Roles should not be empty in WP test environment.' ); + $this->assertArrayHasKey( 'administrator', $roles ); }tests/php/tests/PUM_Analytics_Expanded_Test.php (3)
361-374: Inconsistent assertion approach:get_post_metavs$popup->get_event_count().Other tests in this file (e.g., lines 52, 69, 87) use
$popup->get_event_count('open')to verify counts. This test reaches directly intoget_post_metawith a hard-coded meta keypopup_open_count. If the meta key ever changes, this test would silently pass with stale data while the object API tests would correctly fail.Consider using
$popup->get_event_count('open')for consistency, or at least re-fetching the popup object to avoid any caching:Suggested change
- $count = (int) get_post_meta( $this->popup_id, 'popup_open_count', true ); - $this->assertEquals( 3, $count, 'Open count should be 3 after three tracks.' ); + $popup = pum_get_popup( $this->popup_id ); + $this->assertEquals( 3, $popup->get_event_count( 'open' ), 'Open count should be 3 after three tracks.' );
452-462: Tests correctly document surprisingendpoint_absintbehavior — consider adding doc comments noting this.A function named
absintthat accepts floats (3.14) and negative strings ('-5') as valid is counterintuitive. The tests document this well, but a brief inline note (similar to theBUG:prefix convention used elsewhere in this PR) would help future readers understand this is documenting existing behavior rather than validating expected behavior.
281-287: This test documents a source-code bug but lacks theBUG:prefix.
event_keys('submit')producing'submited'(missing the doubled 't') is a grammatical bug in the source. Per this PR's convention, tests asserting current buggy behavior should be prefixed withBUG:in the test name (liketest_track_with_missing_event_keyon line 351). Consider renaming for consistency:Suggested change
- public function test_event_keys_custom_event_not_ending_in_e() { + public function test_bug_event_keys_custom_event_not_ending_in_e() { $keys = PUM_Analytics::event_keys( 'submit' ); $this->assertEquals( 'submit', $keys[0], 'First key should be the event name.' ); - // rtrim('submit', 'e') = 'submit', then 'submit' . 'ed' = 'submited' (source just appends 'ed'). - $this->assertEquals( 'submited', $keys[1], 'Second key should add ed suffix (no double t).' ); + // BUG: rtrim('submit', 'e') = 'submit', then 'submit' . 'ed' = 'submited' (source just appends 'ed'). + $this->assertEquals( 'submited', $keys[1], 'BUG: Second key should add ed suffix (no double t).' );tests/php/tests/PUM_Utils_Fields_Test.php (1)
722-735: Unused$argsparameter in action callback.The
$argsparameter in the callback is never read. Since you only check whether the hook fired, the parameter can be dropped.Proposed fix
- $callback = function ( $args ) use ( &$hook_called ) { + $callback = function () use ( &$hook_called ) { $hook_called = true; };tests/php/tests/PUM_Utils_ArrayTest.php (1)
886-891: Weak assertion — consider verifying the actual result shape.
assertNotFalseonly proves the method didn't returnfalse, but doesn't validate the output. Ifreplace_keywith a missing key is expected to return the input unchanged, asserting that would be more informative.Suggested improvement
public function test_replace_key_nonexistent() { $input = [ 'a' => 1 ]; $returned = PUM_Utils_Array::replace_key( $input, 'missing', 'new' ); - // The method does not throw, it will attempt array_combine. - $this->assertNotFalse( $returned ); + // When the old key doesn't exist, the array should remain unchanged. + $this->assertIsArray( $returned ); + $this->assertArrayHasKey( 'a', $returned ); + $this->assertSame( 1, $returned['a'] ); }packages/core-data/src/call-to-actions/__tests__/selectors.test.ts (2)
69-80: Consider importinginitialStatefrom constants instead of manually duplicating the state shape.The CTA reducer test (same directory) imports
initialStatefrom../constants, but this selector test manually definesemptyState. If theStateshape changes (e.g., a new required field is added), this manual definition will silently drift out of sync, potentially causing false test results.♻️ Suggested change
-// Base empty state matching initialState shape. -const emptyState: State = { - byId: {}, - allIds: [], - queries: {}, - editorId: undefined, - editedEntities: {}, - editHistory: {}, - editHistoryIndex: {}, - resolutionState: {}, - notices: {}, - errors: { global: null, byId: {} }, -}; +import { initialState } from '../constants'; + +// Use the canonical initial state from constants. +const emptyState: State = initialState;Note: The
initialStateimport would need to be placed with the other imports after the mocks (around line 5-6 area, afterjest.mockcalls).
11-27:applyPatchmock only handles single-level paths — acceptable for current tests but fragile.
op.path.replace( '/', '' )strips only the first/, so a nested path like/settings/typewould produce a flat keysettings/typerather than a nested property update. All current tests use single-level paths so this works, but consider adding a comment noting this limitation to prevent confusion if deeper paths are added later.packages/core-data/src/popups/__tests__/reducer.test.ts (1)
112-120: Test dispatchesRECEIVE_RECORDSbut is nested under theRECEIVE_QUERY_RECORDSdescribe block.Line 115 dispatches
ACTION_TYPES.RECEIVE_RECORDSrather thanACTION_TYPES.RECEIVE_QUERY_RECORDS. While the test assertion itself is valid (verifying thatRECEIVE_RECORDSwithout a query doesn't update queries), it's misleadingly placed inside theRECEIVE_QUERY_RECORDSdescribe block. Consider moving it to theRECEIVE_RECORDSdescribe block above or changing the dispatch toRECEIVE_QUERY_RECORDSwith no query in the payload.packages/core-data/src/popups/__tests__/selectors.test.ts (1)
185-188: Redundantas unknown as numbercast —0is already anumber.The cast was likely copied from the
'new' as unknown as numberpattern on line 177, but0is natively anumberand needs no coercion.♻️ Suggested fix
- const state = stateWith( { editorId: 0 as unknown as number } ); + const state = stateWith( { editorId: 0 } );
| | Area | Source Files | Lines of Code | Test Files | Test Coverage | | ||
| |------|-------------|---------------|------------|---------------| | ||
| | PHP classes/ | 197 | ~15,000+ | 6 | ~3% | | ||
| | PHP includes/ | 85 | ~4,500+ | (above) | - | | ||
| | TS/TSX packages/ | 288 | ~12,000+ | 7 | ~2.4% | | ||
| | Legacy JS assets/ | 79 | ~5,000+ | 2 | ~2.5% | | ||
| | E2E specs | - | - | 3 | ~5% of workflows | | ||
|
|
There was a problem hiding this comment.
Add blank lines around tables to satisfy markdownlint (MD058).
Multiple tables are missing blank lines before/after, which trips MD058. Please add a blank line above and below each table block.
Suggested fix
**Current state**: ~649 source files, 16 test files. Roughly **2.5% test file coverage**.
| Area | Source Files | Lines of Code | Test Files | Test Coverage |
|------|-------------|---------------|------------|---------------|
| PHP classes/ | 197 | ~15,000+ | 6 | ~3% |
| PHP includes/ | 85 | ~4,500+ | (above) | - |
| TS/TSX packages/ | 288 | ~12,000+ | 7 | ~2.4% |
| Legacy JS assets/ | 79 | ~5,000+ | 2 | ~2.5% |
| E2E specs | - | - | 3 | ~5% of workflows |
+Also applies to: 27-36, 37-47, 48-53
🤖 Prompt for AI Agents
In `@claudedocs/testing-gap-analysis.md` around lines 13 - 20, The Markdown tables
in claudedocs/testing-gap-analysis.md are missing blank lines before and after
each table (MD058); edit the file to insert a single blank line immediately
above and immediately below every table block (including the table shown and
those at ranges 27-36, 37-47, 48-53) so each table is separated by blank lines
from surrounding text or other blocks, then run markdownlint to confirm MD058 is
resolved.
- PHP: 334 tests (312 passing, 22 pre-existing failures) across 16 test files - JS/TS: 400 tests (24/24 suites passing) across 20 test files - Fix Jest moduleNameMapper for @popup-maker/* monorepo packages - Expand ConditionCallbacks from 14 to 58 tests covering all 8 methods - Add coverage infrastructure (pcov, phpunit.xml whitelist) - Document 7 bugs found by tests in claudedocs/BUGS-FOUND-BY-TESTS.md - Fix 7 PHP test failures (analytics, options, logging, REST, models)
Add 9 new test files and expand 8 existing ones covering: - Utils: Sanitize, Format, Fields, Options (pure function tests) - Registries: Conditions, Cookies, Triggers - Admin: Settings, Popups (sanitization pipelines) - Expanded: Analytics, Logging, Subscribers, Array, REST Connect Total suite: 866 PHP tests (1432 assertions), 400 JS/TS tests. 48 tests skipped (dist not built, pro-only features, known bugs).
Reverts 4 source code fixes that were bundled with test commits. Tests now document and assert the actual (buggy) behavior with BUG: prefixed test names and comments referencing BUGS-FOUND-BY-TESTS.md. Source reverts: - omit.ts: restore pick-instead-of-omit implementation (#3) - reducer.ts: restore string/number PURGE key mismatch (#1) - reducer.ts: restore RECEIVE_ERROR state mutation (#2) - validation.ts: restore empty title bypass (#4) Also updates bug documentation with 5 new bugs from Batch 2.
- Fix cleanup in Webhook test tearDown - Replace as any with as unknown as Type across TS test files - Fix tautological INVALIDATE_RESOLUTION tests (proper per-ID state) - Modernize phpunit.xml (remove deprecated attrs, filter -> coverage) - Extract taxonomy registration to setUp/tearDown in ConditionCallbacks - DRY REST_Connect_Test with setUp fixtures and helper method - Fix analytics test using wrong option key (pum_settings -> PUM_Utils_Options API) - Strengthen weak assertions (assertIsBool -> assertFalse, assertTrue(true) -> assertIsString) - Use $id in assertion messages for better diagnostics - Remove unused variables and dead code - Fix toBeTruthy/toBeFalsy -> toBe(true)/toBe(false) for boolean returns - Consolidate Bug #12 into Bug #6, add code block language specifiers
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (3)
tests/php/tests/PUM_DB_Subscribers_Test.php (2)
390-395:⚠️ Potential issue | 🟡 MinorThe method names contradict the assertions.
Both methods say
returns_falsebut assertassertTrue(). Rename them or flip the assertions so the names describe the behavior under test.Also applies to: 472-477
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/php/tests/PUM_DB_Subscribers_Test.php` around lines 390 - 395, The test method test_update_negative_id_returns_false is misnamed: it asserts assertTrue($result) while the name says "returns_false"; fix by making the name reflect the actual expectation (e.g., rename to test_update_negative_id_returns_true) or change the assertion to assertFalse($result) and adjust the comment accordingly; apply the same correction to the other conflicting test noted in the review (the test at 472-477) so method names and their assertions are consistent (check references to $this->db->update and assertTrue/assertFalse when making the change).
630-636:⚠️ Potential issue | 🟡 MinorUse
$id1or remove it.Right now the numeric-search test only proves that something matched. Use
$id1in the assertion to prove the expected row was returned, or drop the variable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/php/tests/PUM_DB_Subscribers_Test.php` around lines 630 - 636, The test assigns $id1 from $this->db->insert but never uses it; either remove $id1 or assert that the numeric search returned that specific row. Update the assertion after $results = $this->db->query([ 's' => '42' ]) to verify the returned rows include the inserted ID (e.g. extract the ID/primary key from $results and assert in_array($id1, ...)) so the test proves the expected row from db->insert was matched by db->query; alternatively, delete the unused $id1 assignment if you don't intend to assert it.tests/php/tests/PUM_ConditionCallbacks_Test.php (1)
356-373:⚠️ Potential issue | 🟡 MinorUnregister
pum_test_cptafter this test.
register_post_type()mutates global state, and this custom type is never removed. That can leak into later condition tests through$wp_post_types.Suggested change
$result = PUM_ConditionCallbacks::post_type( $condition ); $this->assertTrue( $result, 'post_index should match on a post type archive page.' ); + + unregister_post_type( 'pum_test_cpt' ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/php/tests/PUM_ConditionCallbacks_Test.php` around lines 356 - 373, The test registers a custom post type pum_test_cpt but never unregisters it, leaking into global $wp_post_types; after creating the post and asserting PUM_ConditionCallbacks::post_type results, call unregister_post_type for 'pum_test_cpt' (or move registration into setUp and unregister in tearDown) to remove the custom type and restore global state so later tests aren't affected.
🧹 Nitpick comments (3)
packages/core-data/src/popups/__tests__/reducer.test.ts (1)
153-170: Add forward-looking expectations alongside BUG characterization testsThese
BUG:tests correctly document current behavior, but they currently pass only when behavior is broken. Consider adding pairedit.todo/it.skiptests asserting the expected fixed behavior so remediation intent is tracked in-suite.Also applies to: 196-214, 260-284
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core-data/src/popups/__tests__/reducer.test.ts` around lines 153 - 170, Add explicit future-facing assertions next to each current "BUG:" test so the suite documents the intended fixed behavior: for the test that uses reducer and ACTION_TYPES.RECEIVE_ERROR (and the other BUG blocks at lines 196-214 and 260-284), add a paired it.todo or it.skip that asserts the original state's errors object is NOT mutated (e.g., errorRef.global remains null) and that the reducer returns a new state/errors object with the updated value; reference the existing reducer function and ACTION_TYPES.RECEIVE_ERROR to locate the code under test and ensure the new test verifies immutability (no direct mutation of baseState.errors) and correct returned payload handling.tests/php/tests/PUM_Cookies_Test.php (1)
314-322: Minor: Unused loop variable$id.The
$idvariable is declared but not used in the loop body. Consider using it in the assertion message for better debugging, or suppress with underscore prefix.Suggested improvement
public function test_dropdown_list_structure() { $list = $this->cookies->dropdown_list(); $this->assertIsArray( $list ); $this->assertNotEmpty( $list ); foreach ( $list as $id => $name ) { - $this->assertIsString( $name, 'Cookie name should be a string.' ); + $this->assertIsString( $name, "Cookie '$id' name should be a string." ); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/php/tests/PUM_Cookies_Test.php` around lines 314 - 322, In test_dropdown_list_structure, the foreach declares $id but never uses it; either include $id in the assertion message (e.g., reference the current key to make failures clearer) or rename it to an intentionally unused variable like $_id to indicate it's unused; update the loop over $this->cookies->dropdown_list() in test_dropdown_list_structure to use one of these approaches so the unused-variable warning is resolved and assertion messages are more informative.tests/php/tests/FormConversionTracking_Test.php (1)
158-176: Action callback should accept both arguments.The
popup_maker/form_conversion_trackedaction fires with two arguments ($popup_id,$event_data) per the production code (context snippet shows line 165-173), but this test callback only captures$popup_id. While the test passes, it doesn't verify the full action signature.Suggested improvement
public function test_track_form_conversion_fires_action() { $fired_popup_id = null; + $fired_args = null; add_action( 'popup_maker/form_conversion_tracked', - function ( $popup_id ) use ( &$fired_popup_id ) { + function ( $popup_id, $args ) use ( &$fired_popup_id, &$fired_args ) { $fired_popup_id = $popup_id; - } + $fired_args = $args; + }, + 10, + 2 ); $this->service->track_form_conversion( [ 'popup_id' => $this->popup_id, 'form_provider' => 'wpforms', ] ); $this->assertEquals( $this->popup_id, $fired_popup_id, 'Action should fire with correct popup ID.' ); + $this->assertIsArray( $fired_args, 'Action should include form args.' ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/php/tests/FormConversionTracking_Test.php` around lines 158 - 176, The test test_track_form_conversion_fires_action subscribes to the 'popup_maker/form_conversion_tracked' action but its callback only accepts one parameter, while the production code fires two ($popup_id, $event_data); update the test's add_action callback in test_track_form_conversion_fires_action to accept both arguments (e.g., function($popup_id, $event_data) use (&$fired_popup_id, &$fired_event_data)) and pass the correct accepted args count to add_action (priority and 2 accepted args), then assert that $fired_popup_id equals $this->popup_id and that $fired_event_data contains expected keys/values (e.g., 'form_provider' => 'wpforms') to fully verify the action signature.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@claudedocs/testing-gap-analysis.md`:
- Around line 64-72: The Markdown tables (e.g., the one starting with "| # |
File | Lines | What Needs Testing | Risk | Testability |" and all other
pipe-table blocks at the listed ranges) are missing blank lines before and after
them which violates MD058; for each table block (including ranges 75-82, 85-93,
105-114, 117-123, 126-133, 145-156, 159-165, 174-187, 252-259, 264-273, 291-296
and any remaining tables) add a single blank line immediately before the table
start and a single blank line immediately after the table end so every table is
separated from surrounding content.
In `@packages/core-data/src/popups/__tests__/reducer.test.ts`:
- Around line 112-117: The test "does not update queries when no query provided"
is dispatching ACTION_TYPES.RECEIVE_RECORDS instead of
ACTION_TYPES.RECEIVE_QUERY_RECORDS, so it never exercises the reducer branch for
RECEIVE_QUERY_RECORDS; update the test to dispatch reducer( initialState, {
type: ACTION_TYPES.RECEIVE_QUERY_RECORDS, payload: { records } } as unknown as
ReducerAction ) (keeping the same records and assertions) so the reducer,
initialState, and the RECEIVE_QUERY_RECORDS path are actually tested.
In `@tests/php/tests/PUM_AnalyticsTEST.php`:
- Around line 32-35: Replace the skip with a hard test failure/assertion:
instead of calling markTestSkipped when pum_is_popup($popup) returns false,
assert that pum_is_popup($popup) is true (or call $this->fail with a descriptive
message) so the test fails when popup initialization regresses; update the
pum_is_popup check in PUM_AnalyticsTEST (remove markTestSkipped and use
$this->assertTrue(pum_is_popup($popup), 'pum_is_popup() returned false — plugin
failed to initialize a published popup.')).
In `@tests/php/tests/PUM_DB_Subscribers_Test.php`:
- Around line 23-26: Tests leave rows in the pum_subscribers table which makes
tests order-dependent; in PUM_DB_Subscribers_Test::setUp() clear the table
before each test using $wpdb (delete all rows from
{$wpdb->prefix}pum_subscribers) and remove any related version/options via
delete_option for the plugin's DB/version option(s), and add a tearDown() method
that repeats the same cleanup (clear pum_subscribers and delete related
version/options) to ensure a clean slate between tests; reference the test class
PUM_DB_Subscribers_Test, its setUp() method and add tearDown() so the cleanup
always runs.
In `@tests/php/tests/PUM_Services_Options_Test.php`:
- Around line 335-355: The test currently documents and asserts the buggy
behavior of update_many; change it to assert the intended contract (empty values
are removed): update the docblock to state that empty values should be removed
(remove the note about the source bug), keep or rename the test method to
something like test_update_many_removes_empty_values, call
$this->options->update_many([ 'willRemove' => '' ]), then assert the key is NOT
present (use $this->assertArrayNotHasKey('willRemove', $stored) or
assertFalse(isset($stored['willRemove']))) and still assert 'stays' remains
unchanged; reference the test method name test_update_many_removes_empty_values
and the update_many method to locate and apply the change.
In `@tests/php/tests/REST_Connect_Test.php`:
- Around line 414-425: The test test_install_route_is_post_only currently only
asserts POST is present; change it to assert the route exposes exactly one
custom method and that it is 'POST' by replacing the assertContains with an
exact equality check (e.g. assertEquals( ['POST'], array_keys(
$install_route[0]['methods'] ) )) so array_keys returns exactly ['POST']; locate
this in test_install_route_is_post_only and make the same change to the
corresponding other test mentioned (the similar test around lines 430-440) to
ensure they assert exact equality rather than membership.
In `@tests/php/tests/REST_ObjectSearch_Test.php`:
- Around line 95-99: The test currently registers a global rest_api_init
callback via add_action( 'rest_api_init', function () use ( $controller ) {
$controller->register_routes(); } ) which leaves a lingering closure and can
cause routes to be re-registered in later tests; instead, call the
controller->register_routes() directly (or if deferred behavior is required, add
and then remove the specific action) so no persistent global hook is left
behind—update the code around the controller and register_routes usage to remove
the temporary rest_api_init closure.
---
Duplicate comments:
In `@tests/php/tests/PUM_ConditionCallbacks_Test.php`:
- Around line 356-373: The test registers a custom post type pum_test_cpt but
never unregisters it, leaking into global $wp_post_types; after creating the
post and asserting PUM_ConditionCallbacks::post_type results, call
unregister_post_type for 'pum_test_cpt' (or move registration into setUp and
unregister in tearDown) to remove the custom type and restore global state so
later tests aren't affected.
In `@tests/php/tests/PUM_DB_Subscribers_Test.php`:
- Around line 390-395: The test method test_update_negative_id_returns_false is
misnamed: it asserts assertTrue($result) while the name says "returns_false";
fix by making the name reflect the actual expectation (e.g., rename to
test_update_negative_id_returns_true) or change the assertion to
assertFalse($result) and adjust the comment accordingly; apply the same
correction to the other conflicting test noted in the review (the test at
472-477) so method names and their assertions are consistent (check references
to $this->db->update and assertTrue/assertFalse when making the change).
- Around line 630-636: The test assigns $id1 from $this->db->insert but never
uses it; either remove $id1 or assert that the numeric search returned that
specific row. Update the assertion after $results = $this->db->query([ 's' =>
'42' ]) to verify the returned rows include the inserted ID (e.g. extract the
ID/primary key from $results and assert in_array($id1, ...)) so the test proves
the expected row from db->insert was matched by db->query; alternatively, delete
the unused $id1 assignment if you don't intend to assert it.
---
Nitpick comments:
In `@packages/core-data/src/popups/__tests__/reducer.test.ts`:
- Around line 153-170: Add explicit future-facing assertions next to each
current "BUG:" test so the suite documents the intended fixed behavior: for the
test that uses reducer and ACTION_TYPES.RECEIVE_ERROR (and the other BUG blocks
at lines 196-214 and 260-284), add a paired it.todo or it.skip that asserts the
original state's errors object is NOT mutated (e.g., errorRef.global remains
null) and that the reducer returns a new state/errors object with the updated
value; reference the existing reducer function and ACTION_TYPES.RECEIVE_ERROR to
locate the code under test and ensure the new test verifies immutability (no
direct mutation of baseState.errors) and correct returned payload handling.
In `@tests/php/tests/FormConversionTracking_Test.php`:
- Around line 158-176: The test test_track_form_conversion_fires_action
subscribes to the 'popup_maker/form_conversion_tracked' action but its callback
only accepts one parameter, while the production code fires two ($popup_id,
$event_data); update the test's add_action callback in
test_track_form_conversion_fires_action to accept both arguments (e.g.,
function($popup_id, $event_data) use (&$fired_popup_id, &$fired_event_data)) and
pass the correct accepted args count to add_action (priority and 2 accepted
args), then assert that $fired_popup_id equals $this->popup_id and that
$fired_event_data contains expected keys/values (e.g., 'form_provider' =>
'wpforms') to fully verify the action signature.
In `@tests/php/tests/PUM_Cookies_Test.php`:
- Around line 314-322: In test_dropdown_list_structure, the foreach declares $id
but never uses it; either include $id in the assertion message (e.g., reference
the current key to make failures clearer) or rename it to an intentionally
unused variable like $_id to indicate it's unused; update the loop over
$this->cookies->dropdown_list() in test_dropdown_list_structure to use one of
these approaches so the unused-variable warning is resolved and assertion
messages are more informative.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8c23cbc5-634a-4f5f-a0a7-904851b754e0
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (51)
claudedocs/BUGS-FOUND-BY-TESTS.mdclaudedocs/testing-gap-analysis.mdcomposer.jsonpackages/components/src/lib/url-control/__tests__/is-url-like.test.tspackages/core-data/src/call-to-actions/__tests__/reducer.test.tspackages/core-data/src/call-to-actions/__tests__/selectors.test.tspackages/core-data/src/call-to-actions/__tests__/validation.test.tspackages/core-data/src/license/__tests__/reducer.test.tspackages/core-data/src/license/__tests__/selectors.test.tspackages/core-data/src/popups/__tests__/reducer.test.tspackages/core-data/src/popups/__tests__/selectors.test.tspackages/core-data/src/popups/__tests__/validation.test.tspackages/core-data/src/settings/__tests__/reducer.test.tspackages/core-data/src/settings/__tests__/selectors.test.tspackages/registry/src/__tests__/index.test.tspackages/utils/src/lib/__tests__/clamp.test.tspackages/utils/src/lib/__tests__/debug.test.tspackages/utils/src/lib/__tests__/noop.test.tspackages/utils/src/lib/__tests__/omit.test.tspackages/utils/src/lib/__tests__/pick.test.tstests/php/config/bootstrap.phptests/php/phpunit.xmltests/php/tests/FormConversionTracking_Test.phptests/php/tests/LinkClickTracking_Test.phptests/php/tests/Logging_Service_Test.phptests/php/tests/PUM_Admin_OnboardingTEST.phptests/php/tests/PUM_Admin_Popups_Test.phptests/php/tests/PUM_Admin_Settings_Test.phptests/php/tests/PUM_AnalyticsTEST.phptests/php/tests/PUM_Analytics_Expanded_Test.phptests/php/tests/PUM_ConditionCallbacks_Test.phptests/php/tests/PUM_Condition_Helpers_Test.phptests/php/tests/PUM_Conditions_Test.phptests/php/tests/PUM_Cookies_Test.phptests/php/tests/PUM_DB_Subscribers_Test.phptests/php/tests/PUM_Models_CallToAction_Test.phptests/php/tests/PUM_Services_Options_Test.phptests/php/tests/PUM_Triggers_Test.phptests/php/tests/PUM_Utils_ArrayTest.phptests/php/tests/PUM_Utils_Fields_Test.phptests/php/tests/PUM_Utils_Format_Test.phptests/php/tests/PUM_Utils_Options_Test.phptests/php/tests/PUM_Utils_Sanitize_Test.phptests/php/tests/PUM_Utils_Test.phptests/php/tests/PopupMakerTEST.phptests/php/tests/REST_Connect_Test.phptests/php/tests/REST_ObjectSearch_Test.phptests/php/tests/RestAPI/Test_License_REST_Endpoints.phptests/php/tests/RestAPI/Test_Webhook_REST_Endpoints.phptests/php/tests/test-pum_utils_array.phptests/unit/jest.config.js
💤 Files with no reviewable changes (1)
- tests/php/tests/test-pum_utils_array.php
✅ Files skipped from review due to trivial changes (15)
- tests/unit/jest.config.js
- tests/php/tests/RestAPI/Test_License_REST_Endpoints.php
- packages/utils/src/lib/tests/noop.test.ts
- tests/php/tests/RestAPI/Test_Webhook_REST_Endpoints.php
- packages/core-data/src/call-to-actions/tests/validation.test.ts
- packages/utils/src/lib/tests/pick.test.ts
- packages/utils/src/lib/tests/clamp.test.ts
- packages/core-data/src/popups/tests/validation.test.ts
- packages/core-data/src/license/tests/reducer.test.ts
- claudedocs/BUGS-FOUND-BY-TESTS.md
- tests/php/tests/PUM_Condition_Helpers_Test.php
- packages/core-data/src/license/tests/selectors.test.ts
- packages/core-data/src/settings/tests/reducer.test.ts
- tests/php/tests/PUM_Utils_Test.php
- packages/core-data/src/call-to-actions/tests/reducer.test.ts
🚧 Files skipped from review as they are similar to previous changes (14)
- composer.json
- packages/utils/src/lib/tests/omit.test.ts
- tests/php/phpunit.xml
- packages/registry/src/tests/index.test.ts
- packages/core-data/src/settings/tests/selectors.test.ts
- tests/php/tests/PUM_Conditions_Test.php
- tests/php/tests/PUM_Utils_Sanitize_Test.php
- packages/utils/src/lib/tests/debug.test.ts
- packages/components/src/lib/url-control/tests/is-url-like.test.ts
- tests/php/tests/PUM_Models_CallToAction_Test.php
- tests/php/tests/PUM_Analytics_Expanded_Test.php
- packages/core-data/src/popups/tests/selectors.test.ts
- tests/php/tests/PUM_Utils_ArrayTest.php
- tests/php/tests/Logging_Service_Test.php
| | # | File | Lines | What Needs Testing | Risk | Testability | | ||
| |---|------|-------|-------------------|------|-------------| | ||
| | 1 | `Services/Connect.php` | 635 | Remote server connection, token validation, webhook auth | 🔴 Critical | Moderate | | ||
| | 2 | `RestAPI/Connect.php` | 584 | Plugin install/upgrade webhooks, auth verification | 🔴 Critical | Moderate | | ||
| | 3 | `DB/Subscribers.php` | 262 | SQL queries, subscriber data CRUD, table creation | 🔴 Critical | Easy | | ||
| | 4 | `Services/License.php` | 1,135 | License validation, activation/deactivation, status checks | 🔴 Critical | Moderate | | ||
| | 5 | `RestAPI/License.php` | 629 | License REST endpoints (partially tested, expand) | 🟡 High | Easy | | ||
| | 6 | `Analytics.php` | 354 | Event tracking, data recording (partially tested, expand) | 🟡 High | Easy | | ||
|
|
There was a problem hiding this comment.
Add blank lines around all remaining tables to satisfy MD058.
Multiple additional tables throughout the document are missing blank lines before and after. Please add a blank line immediately before and after each table block.
Example fix pattern for all tables
**Bottom line**: The codebase has massive testing gaps everywhere. This plan prioritizes by **risk x effort** — what catches the most bugs per test dollar spent.
---
## What IS Currently Tested
+
| Test File | What It Covers | Quality |
|-----------|---------------|---------|
| `test-popup-maker.php` | Plugin instantiation, constants defined | Smoke test only |
...
| `test-webhook-endpoints.php` | Webhook verify security, install validation, endpoint registration | Good security coverage |
+
### JS/TS (7 test files)Apply this pattern (add blank line before and after) to all tables in the document.
Also applies to: 75-82, 85-93, 105-114, 117-123, 126-133, 145-156, 159-165, 174-187, 252-259, 264-273, 291-296
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@claudedocs/testing-gap-analysis.md` around lines 64 - 72, The Markdown tables
(e.g., the one starting with "| # | File | Lines | What Needs Testing | Risk |
Testability |" and all other pipe-table blocks at the listed ranges) are missing
blank lines before and after them which violates MD058; for each table block
(including ranges 75-82, 85-93, 105-114, 117-123, 126-133, 145-156, 159-165,
174-187, 252-259, 264-273, 291-296 and any remaining tables) add a single blank
line immediately before the table start and a single blank line immediately
after the table end so every table is separated from surrounding content.
| it( 'does not update queries when no query provided', () => { | ||
| const records = [ mockPopup( 5 ) ]; | ||
| const state = reducer( initialState, { | ||
| type: ACTION_TYPES.RECEIVE_RECORDS, | ||
| payload: { records }, | ||
| } as unknown as ReducerAction ); |
There was a problem hiding this comment.
RECEIVE_QUERY_RECORDS “no query” case is not exercising that reducer branch
On Line 115, the test dispatches ACTION_TYPES.RECEIVE_RECORDS, so it never validates the RECEIVE_QUERY_RECORDS path without a query.
Suggested patch
it( 'does not update queries when no query provided', () => {
const records = [ mockPopup( 5 ) ];
const state = reducer( initialState, {
- type: ACTION_TYPES.RECEIVE_RECORDS,
+ type: ACTION_TYPES.RECEIVE_QUERY_RECORDS,
payload: { records },
} as unknown as ReducerAction );
expect( state.queries ).toEqual( initialState.queries );
} );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it( 'does not update queries when no query provided', () => { | |
| const records = [ mockPopup( 5 ) ]; | |
| const state = reducer( initialState, { | |
| type: ACTION_TYPES.RECEIVE_RECORDS, | |
| payload: { records }, | |
| } as unknown as ReducerAction ); | |
| it( 'does not update queries when no query provided', () => { | |
| const records = [ mockPopup( 5 ) ]; | |
| const state = reducer( initialState, { | |
| type: ACTION_TYPES.RECEIVE_QUERY_RECORDS, | |
| payload: { records }, | |
| } as unknown as ReducerAction ); | |
| expect( state.queries ).toEqual( initialState.queries ); | |
| } ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core-data/src/popups/__tests__/reducer.test.ts` around lines 112 -
117, The test "does not update queries when no query provided" is dispatching
ACTION_TYPES.RECEIVE_RECORDS instead of ACTION_TYPES.RECEIVE_QUERY_RECORDS, so
it never exercises the reducer branch for RECEIVE_QUERY_RECORDS; update the test
to dispatch reducer( initialState, { type: ACTION_TYPES.RECEIVE_QUERY_RECORDS,
payload: { records } } as unknown as ReducerAction ) (keeping the same records
and assertions) so the reducer, initialState, and the RECEIVE_QUERY_RECORDS path
are actually tested.
| // Verify pum_is_popup succeeds, otherwise tracking will be skipped. | ||
| if ( ! pum_is_popup( $popup ) ) { | ||
| $this->markTestSkipped( 'pum_is_popup() returned false — plugin may not be fully initialized in test context.' ); | ||
| } |
There was a problem hiding this comment.
Fail here instead of skipping.
pum_is_popup() returning false after creating a published popup is exactly the regression this test should catch. markTestSkipped() turns that failure into a green build and hides broken popup initialization.
Suggested change
- // Verify pum_is_popup succeeds, otherwise tracking will be skipped.
- if ( ! pum_is_popup( $popup ) ) {
- $this->markTestSkipped( 'pum_is_popup() returned false — plugin may not be fully initialized in test context.' );
- }
+ $this->assertTrue(
+ pum_is_popup( $popup ),
+ 'Expected pum_get_popup() to return a valid popup model.'
+ );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/php/tests/PUM_AnalyticsTEST.php` around lines 32 - 35, Replace the skip
with a hard test failure/assertion: instead of calling markTestSkipped when
pum_is_popup($popup) returns false, assert that pum_is_popup($popup) is true (or
call $this->fail with a descriptive message) so the test fails when popup
initialization regresses; update the pum_is_popup check in PUM_AnalyticsTEST
(remove markTestSkipped and use $this->assertTrue(pum_is_popup($popup),
'pum_is_popup() returned false — plugin failed to initialize a published
popup.')).
| public function setUp(): void { | ||
| parent::setUp(); | ||
| $this->db = new PUM_DB_Subscribers(); | ||
| } |
There was a problem hiding this comment.
Reset the subscribers table between tests.
This suite writes directly to pum_subscribers, but WP_UnitTestCase will not clean custom tables for you. Exact-count assertions and even the negative-ID cases become order-dependent because earlier rows remain in place. Add setup/teardown cleanup for the table and related version options.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/php/tests/PUM_DB_Subscribers_Test.php` around lines 23 - 26, Tests
leave rows in the pum_subscribers table which makes tests order-dependent; in
PUM_DB_Subscribers_Test::setUp() clear the table before each test using $wpdb
(delete all rows from {$wpdb->prefix}pum_subscribers) and remove any related
version/options via delete_option for the plugin's DB/version option(s), and add
a tearDown() method that repeats the same cleanup (clear pum_subscribers and
delete related version/options) to ensure a clean slate between tests; reference
the test class PUM_DB_Subscribers_Test, its setUp() method and add tearDown() so
the cleanup always runs.
| /** | ||
| * Test update_many with empty values. | ||
| * | ||
| * NOTE: Source has a bug — update_many() unsets the key on line 173 when | ||
| * value is empty, but then unconditionally re-sets it on line 187 with | ||
| * the empty value. So the key persists as empty string. This test asserts | ||
| * actual (buggy) behavior. | ||
| */ | ||
| public function test_update_many_removes_empty_values() { | ||
| update_option( $this->option_key, [ 'willRemove' => 'exists', 'stays' => 'here' ] ); | ||
|
|
||
| $this->options->update_many( [ | ||
| 'willRemove' => '', | ||
| ] ); | ||
|
|
||
| $stored = get_option( $this->option_key ); | ||
| // BUG: key is NOT removed because line 187 re-sets it after unset. | ||
| $this->assertArrayHasKey( 'willRemove', $stored ); | ||
| $this->assertSame( '', $stored['willRemove'] ); | ||
| $this->assertSame( 'here', $stored['stays'] ); | ||
| } |
There was a problem hiding this comment.
Don't make the bug look like the contract.
The docblock and method name say empty values are removed, but the assertions deliberately require the current bug (willRemove still exists). That makes the eventual fix look like a regression and obscures that this is a known-bug characterization.
Suggested change
- /**
- * Test update_many with empty values.
- *
- * NOTE: Source has a bug — update_many() unsets the key on line 173 when
- * value is empty, but then unconditionally re-sets it on line 187 with
- * the empty value. So the key persists as empty string. This test asserts
- * actual (buggy) behavior.
- */
- public function test_update_many_removes_empty_values() {
+ /**
+ * BUG: update_many() currently keeps empty-string values instead of removing them.
+ */
+ public function test_bug_update_many_keeps_empty_values() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/php/tests/PUM_Services_Options_Test.php` around lines 335 - 355, The
test currently documents and asserts the buggy behavior of update_many; change
it to assert the intended contract (empty values are removed): update the
docblock to state that empty values should be removed (remove the note about the
source bug), keep or rename the test method to something like
test_update_many_removes_empty_values, call $this->options->update_many([
'willRemove' => '' ]), then assert the key is NOT present (use
$this->assertArrayNotHasKey('willRemove', $stored) or
assertFalse(isset($stored['willRemove']))) and still assert 'stays' remains
unchanged; reference the test method name test_update_many_removes_empty_values
and the update_many method to locate and apply the change.
| public function test_install_route_is_post_only() { | ||
| [ $controller ] = $this->create_controller_with_mock_service( | ||
| [ 'debug_log', 'get_access_token', 'get_request_token', 'generate_hash', 'debug_mode_enabled' ] | ||
| ); | ||
|
|
||
| $controller->register_routes(); | ||
| $routes = rest_get_server()->get_routes(); | ||
|
|
||
| $install_route = $routes['/popup-maker/v2/connect/install']; | ||
| // The first element should have 'methods' containing POST. | ||
| $this->assertContains( 'POST', array_keys( $install_route[0]['methods'] ), 'Install route should accept POST.' ); | ||
| } |
There was a problem hiding this comment.
These checks don't verify "POST only".
They only prove that POST is one allowed method. If GET or PUT gets added accidentally, both tests still pass. Assert that the route exposes exactly one custom method and that it's POST.
Suggested change
- $this->assertContains( 'POST', array_keys( $install_route[0]['methods'] ), 'Install route should accept POST.' );
+ $this->assertCount( 1, $install_route[0]['methods'] );
+ $this->assertArrayHasKey( 'POST', $install_route[0]['methods'] );
...
- $this->assertContains( 'POST', array_keys( $verify_route[0]['methods'] ), 'Verify route should accept POST.' );
+ $this->assertCount( 1, $verify_route[0]['methods'] );
+ $this->assertArrayHasKey( 'POST', $verify_route[0]['methods'] );Also applies to: 430-440
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/php/tests/REST_Connect_Test.php` around lines 414 - 425, The test
test_install_route_is_post_only currently only asserts POST is present; change
it to assert the route exposes exactly one custom method and that it is 'POST'
by replacing the assertContains with an exact equality check (e.g. assertEquals(
['POST'], array_keys( $install_route[0]['methods'] ) )) so array_keys returns
exactly ['POST']; locate this in test_install_route_is_post_only and make the
same change to the corresponding other test mentioned (the similar test around
lines 430-440) to ensure they assert exact equality rather than membership.
| // Register routes within rest_api_init to avoid WP "incorrect usage" notice. | ||
| $controller = $this->controller; | ||
| add_action( 'rest_api_init', function () use ( $controller ) { | ||
| $controller->register_routes(); | ||
| } ); |
There was a problem hiding this comment.
Remove the temporary rest_api_init callback.
This adds a global hook and never unregisters it. Later REST tests can re-fire rest_api_init and re-register the route through this leftover closure.
Suggested change
- add_action( 'rest_api_init', function () use ( $controller ) {
+ $callback = function () use ( $controller ) {
$controller->register_routes();
- } );
+ };
+ add_action( 'rest_api_init', $callback );
@@
$this->assertArrayHasKey(
'/popup-maker/v2/object-search',
$routes,
'Object search route should be registered.'
);
+
+ remove_action( 'rest_api_init', $callback );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Register routes within rest_api_init to avoid WP "incorrect usage" notice. | |
| $controller = $this->controller; | |
| add_action( 'rest_api_init', function () use ( $controller ) { | |
| $controller->register_routes(); | |
| } ); | |
| // Register routes within rest_api_init to avoid WP "incorrect usage" notice. | |
| $controller = $this->controller; | |
| $callback = function () use ( $controller ) { | |
| $controller->register_routes(); | |
| }; | |
| add_action( 'rest_api_init', $callback ); | |
| $this->assertArrayHasKey( | |
| '/popup-maker/v2/object-search', | |
| $routes, | |
| 'Object search route should be registered.' | |
| ); | |
| remove_action( 'rest_api_init', $callback ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/php/tests/REST_ObjectSearch_Test.php` around lines 95 - 99, The test
currently registers a global rest_api_init callback via add_action(
'rest_api_init', function () use ( $controller ) {
$controller->register_routes(); } ) which leaves a lingering closure and can
cause routes to be re-registered in later tests; instead, call the
controller->register_routes() directly (or if deferred behavior is required, add
and then remove the specific action) so no persistent global hook is left
behind—update the code around the controller and register_routes usage to remove
the temporary rest_api_init closure.
1 participant
Summary
claudedocs/BUGS-FOUND-BY-TESTS.mdcomposer.json)claudedocs/testing-gap-analysis.md)What's covered
PHP (tests/php/tests/)
JS/TS (packages/*/src/tests/)
Bugs documented (not fixed in this PR)
Tests assert current (buggy) behavior with
BUG:prefixed test names. Seeclaudedocs/BUGS-FOUND-BY-TESTS.mdfor full details:Skipped tests (48)
PUM_Admin_Settings:dist/assets/site.cssnot built in test envTest_License_REST_Endpoints: Core class isfinal, can't mockTest_Webhook_REST_Endpoints: Pro-only featureTest plan
Summary by CodeRabbit
Tests
Documentation
Chores