WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#11892 closed defect (bug) (fixed)

Test-Suite: make wp_die() a pluggable function

Reported by: hakre Owned by: westi
Milestone: 3.0 Priority: normal
Severity: blocker Version: 3.0
Component: Unit Tests Keywords:
Focuses: Cc:

Description

There are multiple occurences of the die() (and perhaps exit()) statement within the code.

Those can be a blocker in running the testsuite. I suggest to replace those with a call to a new function called wp_exit(); which is plugable so that the testsuite can replace it.

Otherwise it's not possible to test code containing those statements. At least I'm not aware of a method to do so.

Attachments (2)

11892.patch (1.4 KB) - added by hakre 4 years ago.
Die-Action-Hook with throwing an exception. Does the job.
11892.2.patch (1.5 KB) - added by hakre 4 years ago.
Post-Commit Patch #1

Download all attachments as: .zip

Change History (17)

comment:1 Denis-de-Bernardy4 years ago

  • Milestone changed from Unassigned to 3.0
  • Version set to 3.0

The issue here is that, if it calls die, it's not really meant to continue running the actual code. Wouldn't this make the test suit break? As in, it keeps processing the WP request, and outputs garbage?

comment:2 ramenboy4 years ago

You could throw an exception from a hook to abort the function without terminating execution (assuming you write your tests in PHP5).

comment:3 follow-up: dd324 years ago

What functions can you not test due to this?

The Test suite should be testing business logic, exit shouldnt be called in business logic, the end display logic should be doing that.

comment:4 in reply to: ↑ 3 hakre4 years ago

Replying to Denis-de-Bernardy:

The issue here is that, if it calls die, it's not really meant to continue running the actual code. Wouldn't this make the test suit break?

Yes, that's the problem, the command to die() exits the testsuite and breaks it.


Replying to ramenboy:

You could throw an exception from a hook to abort the function without terminating execution (assuming you write your tests in PHP5).

Yes that's what I wanted to do in the plugable / hooked function wp_exit(); in testing then. So we can even write tests that test for dying by testing for such a special exception. And afaik PHPUnit can deal with exceptions pretty well.


Replying to dd32:

What functions can you not test due to this?

php wp-test.php -t WPTestUserCapabilities

One of the tests results in the call of wp_die() which outputs a message and dies then (You can't give users that role.).

comment:5 hakre4 years ago

wp_die related test that "throws" it is:

    // a role that doesn't exist
    function test_bogus_role()

in WPTestUserCapabilities. We really need to make wp_die hookable for testing. I can offer to write a patch for that.

hakre4 years ago

Die-Action-Hook with throwing an exception. Does the job.

comment:6 westi4 years ago

I think it is better to make wp_die a wrapper function which filters the function name to be called so a plugin can enforce a different one as well as us overloading it for the test cases.

comment:7 automattor4 years ago

(In [12790]) Allow for an alternative handler for wp_die to be used if required. See #11892.

comment:8 hakre4 years ago

For me both ways work, last commit does help with no need to test for a exception but the function wp_die() use must now take care that using wp_die() must not mean that this was the end of the script.

comment:9 hakre4 years ago

I found some time to review this again. Attached you'll find a patch that updates the test_bogus_role and an addition to _dis/_enable_wp_die() so that it can be checked for having died. In this testcase, the test will now fail if the script does not die.

Related: #2914

hakre4 years ago

Post-Commit Patch #1

comment:10 hakre4 years ago

Related: #8770 / [10323].

comment:11 hakre4 years ago

  • Summary changed from replace die() and exit() with a pluggable wp_exit(); function to make wp_die() a pluggable function

comment:12 ShaneF4 years ago

  • Cc ShaneF added

comment:13 hakre4 years ago

I think this is solved now. Westi just added that into the testsuite.

When the changeset is known, this ticket can be closed.

comment:14 hakre4 years ago

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

Changeset is r289 in the testing repository.

enclose parts that die() with:

_disable_wp_die();
_enable_wp_die();

Keep in mind that code which calls wp_die() normally expects that nothing else follows in the execution order. Now it does. Since there was no additional feedback on that within the last weeks, I will close this ticket for now in the lack of traction.

comment:15 hakre4 years ago

  • Summary changed from make wp_die() a pluggable function to Test-Suite: make wp_die() a pluggable function

I'll prefix those tickets in the new report {37} that are related to the suite. This one is a hybrid, but it definitely needs no test, so I prefix it.

Note: See TracTickets for help on using tickets.