Try always using assertSame()#2947
Conversation
|
Here's the code health analysis summary for commits Analysis Summary
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughConfiguration file updated to enforce strict PHPUnit assertion comparisons, followed by systematic replacement of loose equality assertions ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
tests/phpunit/misc/test_FrmSpamCheckDenylist.php (1)
114-146:⚠️ Potential issue | 🔴 CriticalPipeline failure: field IDs are integers in test data but strings from DB queries.
The pipeline confirms this at Line 116:
Expected 460 as int vs 460 as string.$this->factory->field->create()returnsint, butget_field_ids_to_check()returns field IDs from database queries which are strings in PHP/MySQL. This affects all fourassertSamecalls in this method (Lines 114, 126, 140, 146).Cast the factory-created IDs to strings, or use
array_map('intval', ...)on the result:🐛 Proposed fix: cast field IDs to string for comparison
$this->assertSame( array( - $this->text_field_id, - $this->name_field_id, + (string) $this->text_field_id, + (string) $this->name_field_id, ), $field_ids_to_check );Apply the same
(string)cast pattern to all field ID references in Lines 126–134, 140, and 146.tests/phpunit/database/test_FrmMigrate.php (1)
99-131:⚠️ Potential issue | 🔴 CriticalPipeline failure: type mismatch between expected and actual field size values.
The pipeline confirms failure at Line 129:
Failed asserting that 10 is identical to '10'. After unserialization,field_options['size']can be returned asintwhen the value is purely numeric, while$expected($sizesvalues) are strings or the result ofround().Two issues:
'10' => '10'— DB unserialization may yieldint 10instead ofstring '10'.'1024px' => round(1024 / 9)—round()returnsfloat/int, but the DB will store and return a string.🐛 Proposed fix: ensure consistent types
$sizes = array( '10px' => '10px', - '10' => '10', - '1024' => '1024', - '1024px' => round( 1024 / 9 ), + '10' => 10, + '1024' => 1024, + '1024px' => (int) round( 1024 / 9 ), );Alternatively, cast
$new_sizeto string before assertion if the expected values should remain strings, or cast to(string)onround():- '1024px' => round( 1024 / 9 ), + '1024px' => (string) round( 1024 / 9 ),and investigate why
'10'comes back asintfrom unserialization to pick the right fix direction.tests/phpunit/fields/test_FrmFieldType.php (1)
119-138:⚠️ Potential issue | 🔴 CriticalPipeline failure:
user_idsanitization returns integers, but expected values are strings.The pipeline confirms at Line 137: expected
['6','2','0'](strings) but got[6,2,0](ints). Theuser_idfield type'ssanitize_valueappliesabsint/intval, converting values to integers, while the test's expected array uses string literals.🐛 Proposed fix: update expected values to match sanitized types
array( 'type' => 'user_id', 'value' => array( '6', '2a', 'a1', ), 'expected' => array( - '6', - '2', - '0', + 6, + 2, + 0, ), ),tests/phpunit/emails/test_FrmEmail.php (1)
764-769:⚠️ Potential issue | 🔴 CriticalOnly
include_user_infohas a type mismatch;is_plain_textis handled correctly.
test_set_include_user_infoexpects boolean values (false/true), butFrmEmail::set_include_user_info()at line 302 directly assigns$this->settings['inc_user_info']without converting the string values'0'/'1'from the test. The property stores'0'and'1'(strings), causingassertSame(false, '0')to fail.In contrast,
test_set_is_plain_textshould pass becauseFrmEmail::set_is_plain_text()correctly converts to boolean via! empty()at line 285:! empty('0')evaluates tofalseand! empty('1')evaluates totrue.🐛 Fix for `test_set_include_user_info`
Either cast expected values to strings or change to
assertEquals:Option A: Update expected values to match actual stored types:
public function test_set_include_user_info() { $settings = array( - '0' => false, - '1' => true, + '0' => '0', + '1' => '1', ); $this->check_private_properties( $settings, 'inc_user_info', 'include_user_info' ); }Option B: Use
assertEqualsfor loose type comparison.
🤖 Fix all issues with AI agents
In `@tests/phpunit/database/test_FrmMigrate.php`:
- Around line 68-93: The size assertions in test_migrate_to_17 risk failing due
to type differences (string vs int) for $field->field_options['size']; change
the assertions to use a type-flexible comparison or normalize types before
asserting: for example, in test_migrate_to_17 convert both $expected_size and
$field->field_options['size'] to the same type (cast to string or int) or
replace assertSame with assertEquals when checking $field->field_options['size']
after calls to FrmField::update and $frmdb->upgrade so the test no longer fails
if unserialization returns int 10 instead of string '10'.
In `@tests/phpunit/emails/test_FrmEmail.php`:
- Line 749: The test assertion uses PHPUnit's assertSame with arguments
reversed; change the call to assertSame so the expected value is first and the
actual value second — replace $this->assertSame( $actual, $expected ) with
$this->assertSame( $expected, $actual ) in the test (look for the assertSame
invocation using the $actual and $expected variables in test_FrmEmail.php).
In `@tests/phpunit/entries/test_FrmEntriesController.php`:
- Line 64: The assertion fails due to type mismatch between $entry->post_id
(string from DB) and $new_post->ID (int); update the test assertion in
test_FrmEntriesController.php to cast the DB value to int (compare
(int)$entry->post_id to $new_post->ID) so types match and the assertSame passes.
In `@tests/phpunit/fields/test_FrmFieldNumber.php`:
- Around line 16-19: The test is failing because check_value_is_valid_with_step
returns float values but the assertions use integer literals; update the
expected arrays in the assertions to use float values so types match (e.g., 0.0,
3.0 and 8.0, 10.0) for the calls to
$number_field->check_value_is_valid_with_step (including the string-input case
to assert it still returns floats); keep using assertSame to ensure type
equality with the returned float array.
In `@tests/phpunit/fields/test_FrmFieldsController.php`:
- Line 87: The failing assertion uses assertSame comparing $form_id (int) to
$new_field['form_id'] (string); update the test in test_FrmFieldsController.php
to avoid the strict type mismatch by either casting $new_field['form_id'] to int
before the assertSame or replace this single assertion with
$this->assertEquals($form_id, $new_field['form_id']); locate the assertion using
the symbols $form_id, $new_field['form_id'], and assertSame and apply one of
these two fixes so the types match.
In `@tests/phpunit/fields/test_FrmFieldsHelper.php`:
- Around line 216-227: The tests fail due to strict type mismatch between
integer IDs returned by $this->factory->field->create() (e.g., $draft_field_id,
$draft_field_id2) and the string IDs returned by wp_list_pluck($results, 'id');
fix by normalizing types before the assertions: convert the plucked $ids to
integers (e.g., $ids = array_map('intval', $ids)) prior to the assertSame calls
that compare to $draft_field_id/$draft_field_id2 (or alternatively cast
$draft_field_id/$draft_field_id2 to strings), ensuring
FrmFieldsHelper::get_draft_field_results test assertions compare identical
types.
In `@tests/phpunit/form-templates/test_FrmFormTemplatesController.php`:
- Line 139: The test uses $this->assertSame($expected, $favorites, ...) which
fails due to associative array key order; change the assertion to
$this->assertEquals($expected, $favorites, ...) so the comparison ignores key
order, or alternatively normalize both arrays before asserting (e.g.,
ksort($expected); ksort($favorites);) — update the assertion in
test_FrmFormTemplatesController (the line with $this->assertSame and variables
$expected and $favorites) accordingly.
In `@tests/phpunit/misc/test_FrmAppHelper.php`:
- Line 410: The test is using strict type comparison: replace the assertion that
uses assertSame comparing $new_post_id (from go_to_new_post()) and $get_post_id
(from get_query_var()) with assertEquals so the numeric string vs integer
comparison succeeds; update the assertion call from $this->assertSame(
$new_post_id, $get_post_id ); to use $this->assertEquals( $new_post_id,
$get_post_id ); ensuring the test still verifies equality but not strict type
identity.
In `@tests/phpunit/stripe/test_FrmTransLiteActionsController.php`:
- Line 38: The test fails due to a type mismatch between $field_id (int returned
by $this->factory->field->create()) and $field (string returned by
get_fields_for_price()); to fix, make the types match in the test by casting the
expected value to string before asserting (change the assertion around
$this->assertSame( $field_id, $field ) to use a string-cast of $field_id), or
alternatively update get_fields_for_price() to return ints—prefer the quick
test-side fix by casting $field_id to (string) so $field and $field_id have the
same type.
🧹 Nitpick comments (2)
tests/phpunit/entries/test_FrmTableHTMLGenerator.php (1)
57-60: Nit:assertSameargument order is reversed (pre-existing).PHPUnit convention is
assertSame($expected, $actual). On lines 57 and 60, the method call result (actual) is the first argument and$html(expected) is second. This won't affect correctness forassertSamewith strings, but it will produce a confusing diff message on failure. This was pre-existing from theassertEqualsversion, so low priority.Suggested fix
- $this->assertSame( $table_generator->remove_border( $html, 'bottom' ), $html ); + $this->assertSame( $html, $table_generator->remove_border( $html, 'bottom' ) ); $html = '<div style="border-top:1px solid `#eee`;"></div>'; - $this->assertSame( $table_generator->remove_border( $html ), $html ); + $this->assertSame( $html, $table_generator->remove_border( $html ) );tests/phpunit/emails/test_FrmEmailStylesController.php (1)
9-33: Arguments are in reversed order across all three assertions.PHPUnit's
assertSame($expected, $actual)expects the expected value first. Here, the method result (actual) is passed as the first argument and$new_content(expected) is the second. This won't cause failures but will produce confusing messages if a test fails.Proposed fix: swap argument order
$this->assertSame( - $this->run_private_method( - array( 'FrmEmailStylesController', 'add_inline_css' ), - array( 'div', $css, $content ) - ), - $new_content + $new_content, + $this->run_private_method( + array( 'FrmEmailStylesController', 'add_inline_css' ), + array( 'div', $css, $content ) + ) );Same for the
'p'and'a'assertions below.
Try always using assertSame()
1 participant
This update changes most uses of
assertEqualstoassertSame, but not all (where it is failing).I updated some tests but left several for now.
Summary by CodeRabbit
Release Notes
Tests
Chores