WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#36166 closed defect (bug) (fixed)

wp_die() doesn't accept a WP_Error during unit testing

Reported by: dd32 Owned by: boonebgorges
Milestone: 4.5 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: has-patch
Focuses: Cc:

Description

As reported on the forums: https://wordpress.org/support/topic/passing-wp_error-instance-to-wp_die-when-unit-testing-doesnt-work?replies=2

I'm testing some code using PHPUnit, and when my code uses
wp_die(new WP_Error('errorCode', 'errorMessage'));
I get the error:
Error: Wrong parameters for WPDieException([string $message [, long $code [, Throwable $previous = NULL]]])
If I change my code to:
wp_die('errorMessage');
Then it works correctly (I get WPDieException: errorMessage).

The unit test wp_die() handler should accept the same things that the default wp_die() handler can.

Attachments (3)

36166.patch (741 bytes) - added by utkarshpatel 3 years ago.
Handle WP_Error object in default _ajax_wp_die_handler
36166_1.patch (648 bytes) - added by utkarshpatel 3 years ago.
remove switch case from previous patch
36166_2.patch (539 bytes) - added by utkarshpatel 3 years ago.
Enhancement of wp_die_handler function in test case which accepts wp_error object as a param.

Download all attachments as: .zip

Change History (11)

#1 @utkarshpatel
3 years ago

  • Keywords has-patch added; needs-patch removed

I debug the issue and found when we call wp_die() from PHPunit it is going inside the default die handler of ajax _ajax_wp_die_handler and that function is not designed to handle WP_Error object.

I don't know if this is right way but here i've handled WP_Error in _ajax_wp_die_handler()

@utkarshpatel
3 years ago

Handle WP_Error object in default _ajax_wp_die_handler

@utkarshpatel
3 years ago

remove switch case from previous patch

This ticket was mentioned in Slack in #core by patelutkarsh. View the logs.


3 years ago

#3 follow-up: @boonebgorges
3 years ago

Could I get some more details on how to reproduce? It seems as if the issue being reported by @xoogu in the support forums is somewhat different from what's being proposed by @utkarshpatel. @dd32 suggests that the AJAX die handler for unit tests should match the src handler. But this should already be the case:

In each case, a scalar value is handled one way (printing or passing to the Exception), while all other values die with '0'.

36166_1.patch proposes an enhancement that passes custom WP_Error messages to the die handler. This may be a viable enhancement, but it doesn't address the original report, which suggests that errors are handled differently in the context of unit tests.

Possibly related, AJAX test classes are responsible for providing their own die handlers. Core tests do this by extending WP_Ajax_UnitTestCase. If @utkarshpatel's tests are invoking the default _ajax_wp_die_handler(), I'm guessing that those tests are *not* extending WP_Ajax_UnitTestCase.

#4 in reply to: ↑ 3 @xoogu
3 years ago

Replying to boonebgorges:

Could I get some more details on how to reproduce? It seems as if the issue being reported by @xoogu in the support forums is somewhat different from what's being proposed by @utkarshpatel.

The issue I was having was when installing a plugin as part of the initial setup for testing. (The plugin install was called from setUpBeforeClass of my test). If the plugin couldn't be installed, then wp_die was called with a WP_Error object passed to it.

(I realise requiring another plugin to be installed rather than just mocking it isn't actually unit testing, but that's just how I like to do my tests).

#5 follow-up: @dd32
3 years ago

To test it, I simply added that example to the functions.php unit tests:

+	function test_wp_die_with_wp_error() {
+		wp_die( new WP_Error( 'test', 'test' ) );
+	}

and got the fatal error as stated.

It turns out that the Ajax wp_die() handler handles it correctly, but the WP_UnitTest wp_die handler doesn't: https://core.trac.wordpress.org/browser/tags/4.4.2/tests/phpunit/includes/testcase.php#L274

@utkarshpatel
3 years ago

Enhancement of wp_die_handler function in test case which accepts wp_error object as a param.

#6 @utkarshpatel
3 years ago

@boonebgorges yes, it was mistake but code will be same and we can also use enhancement of 36166_1.patch

Changes on https://core.trac.wordpress.org/browser/tags/4.4.2/tests/phpunit/includes/testcase.php#L274 function are attached in 36166_2.patch

#7 in reply to: ↑ 5 @boonebgorges
3 years ago

  • Milestone changed from Awaiting Review to 4.5

Replying to dd32:

To test it, I simply added that example to the functions.php unit tests:

+	function test_wp_die_with_wp_error() {
+		wp_die( new WP_Error( 'test', 'test' ) );
+	}

and got the fatal error as stated.

It turns out that the Ajax wp_die() handler handles it correctly, but the WP_UnitTest wp_die handler doesn't: https://core.trac.wordpress.org/browser/tags/4.4.2/tests/phpunit/includes/testcase.php#L274

Ah yes. Thanks for the pointer. This is what I was guessing at in this comment.

@utkarshpatel The proposal that wp_die() print WP_Error messages is worth considering, but it's beyond the scope of this ticket. The proposal raises a number of concerns related to backward compatibility, empty messages, and other potential problems. If you'd like to consider it, I encourage you to open an enhancement ticket.

#8 @boonebgorges
3 years ago

  • Owner set to boonebgorges
  • Resolution set to fixed
  • Status changed from new to closed

In 37071:

Tests: Ensure that the default wp_die() handler can handle a WP_Error object.

Props dd32, utkarshpatel.
Fixes #36166.

Note: See TracTickets for help on using tickets.