Add post password length validation to Quick Edit and server-side processing #9776
Conversation
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
SirLouen
left a comment
SirLouen
left a comment
There was a problem hiding this comment.
I would also add the @covers to the test__wp_translate_postdata_cap_checks_contributor test.
I always like to correct some extra tests when I'm doing something similar if you have the opportunity. Some tests are old and not
| * Test that _wp_translate_postdata() validates post password length. | ||
| * | ||
| * @ticket 63943 | ||
| */ |
There was a problem hiding this comment.
Add a @covers ::_wp_translate_postdata. We have to get used to add this
| * | ||
| * @ticket 63943 | ||
| */ | ||
| public function test__wp_translate_postdata_validates_post_password_length() { |
There was a problem hiding this comment.
I don't love the function name with the new format
maybe something like test_invalid_length_post_password would be better.
| $result = _wp_translate_postdata( false, $post_data ); | ||
| $this->assertWPError( $result ); | ||
| $this->assertSame( 'invalid_post_password_length', $result->get_error_code() ); | ||
| $this->assertSame( 'Post passwords cannot be longer than 255 characters.', $result->get_error_message() ); |
There was a problem hiding this comment.
You don't need 3 asserts to test the exact same element. This is what I told you in the review. Get used to cut down the fluff when you use AI for creating tests. 80% of the times unit tests with AI are terrible and the other 20% like this case are good but with a ton of fluff. Be careful with this, in either cases you cannot ever leave the tests untouched. It's way better to try to build your own test, and fail to do so (or be unable to do it which is fine also), but at least fail trying, than just adding a wrong test made with AI. Just have this in mind.
There was a problem hiding this comment.
Thanks for the feedback!
I usually check the existing test cases in the codebase to follow a similar structure. While going through the code, I found a similar pattern being used, which is why I followed that approach in my PR (for example: core test reference).
The same goes for the function naming, I kept it consistent with the existing test functions in that file. That said, I’ll keep your point in mind about cutting down extra assertions.
There was a problem hiding this comment.
You can use a similar pattern for the first test you removed.
But for the second one, it's too repetitive: Check if there is an error, check if the error has this code, check if the error has this message. 3 times checking for the exact same thing from different angles.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
| * @ticket 63943 | ||
| * @covers ::_wp_translate_postdata | ||
| */ | ||
| public function test_invalid_length_post_password() { |
There was a problem hiding this comment.
If we are going to set both tests in the same test function, then we should set a better naming.
Maybe test_validity_post_password_length
2 participants
Trac ticket: https://core.trac.wordpress.org/ticket/63943