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

test for wp_maybe_load_widgets #3682

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

pbearne
Copy link

@pbearne pbearne commented Nov 25, 2022

Copy link
Contributor

@costdev costdev left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @pbearne and for working to implement standards early to save time in reviewing / implementing review feedback! Much appreciated and look forward to seeing that in future PRs!

I've left some thoughts below which should help with an unmet standard (message parameter) and test stability. 🙂

* @group functions.php
*
* @covers ::wp_maybe_load_widgets
*/#
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
*/#
*/

There was just a stray # here

* @ticket 57201
*/
public function test_wp_maybe_load_widgets() {
$this->assertFalse( class_exists( 'WP_Nav_Menu_Widget' ) );
Copy link
Contributor

Choose a reason for hiding this comment

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

A couple of thoughts:

  1. Test methods with multiple assertions should have a message parameter to explain what went wrong.

For similar assertions (i.e. multiple assertion of class_exists()), the message can be:

  • WP_Nav_Menu_Widget should not exist before doing something.
  • WP_Nav_Menu_Widget should not exist after doing something.
  • WP_Nav_Menu_Widget should exist after doing something.
  1. It may be best to split this into two test methods:
  • test_wp_maybe_load_widgets_should_respect_falsy_load_default_widgets_filter() - Use a data provider with falsy values to send to the filter hook via static closure, then assertFalse() for class_exists() and has_action().
  • test_wp_maybe_load_widgets_should_load_widgets() - assertFalse() for class_exists() and has_action(), then call wp_maybe_load_widgets(), then assertTrue() on class_exists() and has_action() (all assertions should have the message parameter)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants