Opened 3 years ago
#53842 new defect (bug)
Review the type of select return values
Reported by: | jrf | Owned by: | |
---|---|---|---|
Milestone: | Awaiting Review | Priority: | normal |
Severity: | normal | Version: | |
Component: | Build/Test Tools | Keywords: | 2nd-opinion |
Focuses: | rest-api | Cc: |
Description
In addressing ticket #46149, four tests were encountered which were testing (part of) a return value of WP Core functions, but were found to be using the wrong value type in this comparison.
This was discovered due to the fact that the assertContains()
method in PHPUnit uses strict type comparisons as of PHPUnit 8.0.2.
For the time being, the tests will be updated to reflect the REAL return type per proposed commit https://github.com/jrfnl/wordpress-develop-official/commit/64dd09d5c292c3eac80419d5a1a325dab453a5ed
This ticket is being opened to follow up on this as it should be investigated whether the return type of these WP Core function as-is is actually correct and the test update was therefore justified. Or whether the test update should be reverted and the actual WP Core functions should be updated.
Details
Tests_Comment_GetPageOfComment::test_page_number_when_unapproved_comments_are_included_for_current_commenter()
and Tests_Comment_GetPageOfComment::test_page_number_when_unapproved_comments_are_included_for_current_user()
Both these tests generate a comment ID using a test Factory class:
<?php $new_unapproved = self::factory()->comment->create( $comment_args );
$new_unapproved
will now contain a comment ID as an integer.
This integer is expected to be included in a list retrieved via the `get_comments()` function and filtered via `wp_list_pluck()`.
Based on these two tests, the comment_ID
as returned in the array retrieved via get_comments()
is a string, not an integer.
So the questions for these two tests are:
- Should the
comment_ID
returned byget_comments()
be a string or an integer ? - If the
get_comments()
function should remain unchanged, should the TestCase::factory()->comment->create()` method be adjusted to return a string for the ID value instead of an integer ? - If the answer to both the above questions is "no", no further action is needed once the proposed commit mentioned above has been committed.
WP_Test_REST_Post_Meta_Fields::test_set_value_multiple_custom_schema()
and WP_Test_REST_Term_Meta_Fields::test_set_value_multiple_custom_schema()
These two tests both update the post meta of a WP post via a REST API POST request.
The data to be set is passed as integers:
<?php $data = array( 'meta' => array( 'test_custom_schema_multi' => array( 2, 8 ), ), ); $request->set_body_params( $data ); $response = rest_get_server()->dispatch( $request );
The test subsequently requests the post meta information via a call to `get_post_meta()` and ensures that the returned array contains both values.
The values returned in the array from get_post_meta()
, however are strings, not integers.
So the questions for these two tests are:
- Where does the type change from integer to string happen ?
- Is the data when set via the REST API being saved correctly ?
- Does the Core code need to change or is the test update correct ?
In this case, I suspect the type change may be due to the fact that all data received from $_POST
will always be in string format, however, the handling of these API requests should be investigated to be sure.