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

Fix PHP Tests: Try removing registry from WP_Block constructor calls #43927

Merged
merged 1 commit into from
Sep 7, 2022

Conversation

andrewserong
Copy link
Contributor

@andrewserong andrewserong commented Sep 7, 2022

What?

Try to fix the following errors by removing passing in a registry for these four tests:

There were 4 errors:

1) Render_Block_Navigation_Test::test_block_core_navigation_get_post_ids_from_block
Undefined property: Render_Block_Navigation_Test::$registry

/var/www/html/wp-content/plugins/gutenberg/phpunit/blocks/render-block-navigation-test.php:24
phpvfscomposer:///var/www/html/wp-content/plugins/gutenberg/vendor/phpunit/phpunit/phpunit:97

2) Render_Block_Navigation_Test::test_block_core_navigation_get_post_ids_from_block_nested
Undefined property: Render_Block_Navigation_Test::$registry

/var/www/html/wp-content/plugins/gutenberg/phpunit/blocks/render-block-navigation-test.php:[49](https://github.com/WordPress/gutenberg/runs/8218697872?check_suite_focus=true#step:8:50)
phpvfscomposer:///var/www/html/wp-content/plugins/gutenberg/vendor/phpunit/phpunit/phpunit:97

3) Render_Block_Navigation_Test::test_block_core_navigation_get_post_ids_from_block_with_submenu
Undefined property: Render_Block_Navigation_Test::$registry

/var/www/html/wp-content/plugins/gutenberg/phpunit/blocks/render-block-navigation-test.php:62
phpvfscomposer:///var/www/html/wp-content/plugins/gutenberg/vendor/phpunit/phpunit/phpunit:97

4) Tests_Blocks_RenderComments::test_render_block_core_comments_empty_output_if_comments_disabled
Undefined property: Tests_Blocks_RenderComments::$registry

/var/www/html/wp-content/plugins/gutenberg/phpunit/blocks/render-comments-test.php:41
phpvfscomposer:///var/www/html/wp-content/plugins/gutenberg/vendor/phpunit/phpunit/phpunit:97

Why?

Tests are currently failing on trunk and I'm not sure if we ever needed to pass in the registry for these tests 🤔

I suspect that something upstream means that the registry isn't available in these tests, however from a quick read of the tests, it doesn't look to me like they ever really needed a registry, since the tests are looking at rendered markup rather than testing anything about the registry in particular. But I very well could be missing something!

How?

Remove $this->registry from the calls to the WP_Block constructor

Testing Instructions

See if Unit Tests / PHP (pull_request) CI jobs pass

Screenshots or screencast

@andrewserong andrewserong self-assigned this Sep 7, 2022
@andrewserong andrewserong changed the title PHP Tests: Try removing registry from WP_Block constructor calls Fix PHP Tests: Try removing registry from WP_Block constructor calls Sep 7, 2022
Copy link
Contributor

@talldan talldan left a comment

Choose a reason for hiding this comment

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

LGTM.

I think it was probably this change that caused the test failures, but I haven't looked too deeply - WordPress/wordpress-develop@12f5012.

The failing Gutenberg tests were probably inspired by these ones - https://github.com/WordPress/wordpress-develop/blob/trunk/tests/phpunit/tests/blocks/wpBlock.php.

The core tests do instantiate a $this->registry in the setup function, while the failing tests in Gutenberg don't. I think it's a copy/paste issue.

The way the tests are written I don't think the absence of a registry will cause an issue, but it is noticeable that the comments test calls gutenberg_render_block_core_comments, when debatably it could use $block->render(), and calling that render function would (I think) require a block registry.

Pinging @ockham, @spacedmonkey and @costdev, who seem to have been involved in the introduction of these tests. I leave it up to you if you want to make any further improvements.

@talldan talldan merged commit 0f20196 into trunk Sep 7, 2022
@talldan talldan deleted the try/fix-failing-php-unit-tests branch September 7, 2022 05:15
@github-actions github-actions bot added this to the Gutenberg 14.1 milestone Sep 7, 2022
@talldan talldan added the [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. label Sep 7, 2022
@andrewserong andrewserong added the [Type] Code Quality Issues or PRs that relate to code quality label Sep 7, 2022
Copy link
Member

@ramonjd ramonjd left a comment

Choose a reason for hiding this comment

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

Thanks @andrewserong !

I think we should get this in and ask for forgiveness later. The pipeline is blocked.

@@ -21,7 +21,7 @@ public function test_block_core_navigation_get_post_ids_from_block() {
);
$parsed_block = $parsed_blocks[0];
$context = array();
$block = new WP_Block( $parsed_block, $context, $this->registry );
$block = new WP_Block( $parsed_block, $context );
Copy link
Member

Choose a reason for hiding this comment

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

Interesting. It looks like the third argument has to be an instance of WP_Block_Type_Registry

Maybe #40752 was copying examples from core tests and not assigning $this->registry a value.

Regardless, maybe another approach would be to set it like the Tests_Blocks_wpBlock test does:

	/**
	 * Set up each test method.
	 */
	public function set_up() {
		parent::set_up();

		$this->registry = new WP_Block_Type_Registry();
	}

Even so it doesn't seem to affect the tests results.

Copy link
Member

Choose a reason for hiding this comment

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

basically what @talldan said 2 minutes ago 🤣

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I appreciate the extra pair of eyes, cheers! 😀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants