Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tests: fix broken setup of mocked WP_Role objects + fix an incorrect test #165

Merged
merged 2 commits into from
Mar 31, 2021

Conversation

jrfnl
Copy link
Contributor

@jrfnl jrfnl commented Mar 31, 2021

Context

  • Improve test suite

Summary

This PR can be summarized in the following changelog entry:

  • Improve test suite

Relevant technical choices:

Tests: fix broken setup of mocked WP_Role objects

This fixes three different issues with the mocking of the WP_Role objects.

1. Incorrect implementation of allows().

As per the Mockery documentation about the allows() syntax, the use of allows() can take two forms:

  1. Passing it an array with method_name => return_value entries.
  2. Not passing it any arguments, but chaining method_name and andReturn( 'return value' ) to it.

In the Duplicate Post TestCase class method [1] was being used, but implemented incorrectly.

The return_value should be an actual value, not a callback which will be called to get at the value.

In practice, that meant that the mocked WP_Role hap_cap(), add_cap() and remove_cap() methods were returning a closure (object instance of the Closure class and not the expected return value.
The only reason this hadn't led to failing tests before is the prevalence of implicit loose type comparisons in the code base, i.e. if ( $something ) {}.

Ref: http://docs.mockery.io/en/latest/reference/alternative_should_receive_syntax.html#allows

2. Void method(s) returning a value.

The WP WP_Role::add_cap() method is, just like the WP_Role::remove_cap() method, a void method, i.e. it does not return a value.

As it was, the add_cap() was returning a closure, but based on the return value of the closure, it was intended for the method to return true.

Changing this to null now in an attempt to more closely match the original method.

Ref: https://developer.wordpress.org/reference/classes/wp_role/add_cap/

3. Incorrect use of makePartial().

As per the documentation:

Partial doubles are useful when we want to stub out, set expectations for, or spy on some methods of a class, but run the actual code for other methods.

As the actual WP_Role class is not available to the test suite as WP will not be loaded, making these mocks "partials" is useless.

Ref: http://docs.mockery.io/en/latest/reference/creating_test_doubles.html#partial-test-doubles

Tests: fix bug in Options_Form_Generator_Test::test_generate_roles_permission_list()

As per the mock of the WP_Role objects in the Yoast\WP\Duplicate_Post\Tests\TestCase::setUp() method, the has_cap() method will respectively return true for the Editor role and false for the Administrator and the Subscriber role.

Based on that, the expected output for the Options_Form_Generator_Test::test_generate_roles_permission_list() test should have only the Editor checkbox checked.

This fixes the incorrectly checked checkbox for the Administrator role.

Test instructions

This PR can be tested by following these steps:

  • N/A This is a test-only change and should have no effect on the functionality. If the build passes (test runs), we're good.

jrfnl added 2 commits March 31, 2021 16:11
This fixes three different issues with the mocking of the WP_Role objects.

1. Incorrect implementation of `allows()`.

As per the Mockery documentation about the `allows()` syntax, the use of `allows()` can take two forms:
1. Passing it an array with `method_name` => `return_value` entries.
2. Not passing it any arguments, but chaining `method_name` and `andReturn( 'return value' )` to it.

In the Duplicate Post `TestCase` class method [1] was being used, but implemented incorrectly.

The `return_value` should be an actual value, not a callback which will be called to get at the value.

In practice, that meant that the mocked `WP_Role` `hap_cap()`, `add_cap()` and `remove_cap()` methods were returning a closure (object instance of the `Closure` class and not the expected return value.
The only reason this hadn't led to failing tests before is the prevalence of implicit loose type comparisons in the code base, i.e. `if ( $something ) {}`.

Ref: http://docs.mockery.io/en/latest/reference/alternative_should_receive_syntax.html#allows

2. Void method(s) returning a value.

The WP `WP_Role::add_cap()` method is, just like the `WP_Role::remove_cap()` method, a `void` method, i.e. it does not return a value.

As it was, the `add_cap()` was returning a closure, but based on the return value of the closure, it was intended for the method to return `true`.

Changing this to `null` now in an attempt to more closely match the original method.

Ref: https://developer.wordpress.org/reference/classes/wp_role/add_cap/

3. Incorrect use of `makePartial()`.

As per the documentation:

> Partial doubles are useful when we want to stub out, set expectations for, or spy on some methods of a class, but run the actual code for other methods.

As the actual `WP_Role` class is not available to the test suite as WP will not be loaded, making these mocks "partials" is useless.

Ref: http://docs.mockery.io/en/latest/reference/creating_test_doubles.html#partial-test-doubles
…rmission_list()

As per the mock of the WP_Role objects in the `Yoast\WP\Duplicate_Post\Tests\TestCase::setUp()` method, the `has_cap()` method will respectively return `true` for the `Editor` role and `false` for the `Administrator` and the `Subscriber` role.

Based on that, the expected output for the `Options_Form_Generator_Test::test_generate_roles_permission_list()` test should have only the `Editor` checkbox checked.

This fixes the incorrectly checked checkbox for the `Administrator` role.
Copy link
Member

@enricobattocchi enricobattocchi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All is clear now, thanks for the explanation! Merging

@enricobattocchi enricobattocchi merged commit c8bd49d into develop Mar 31, 2021
@enricobattocchi enricobattocchi deleted the JRF/Test/fix-broken-wp-role-mocking branch March 31, 2021 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants