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

Copy failing WP_HTML_Tag_Processor_Bookmark_Test tests from Core #47720

Merged
merged 2 commits into from
Feb 3, 2023

Conversation

noisysocks
Copy link
Member

PHP unit tests are failing on trunk:

https://github.com/WordPress/gutenberg/actions/runs/4080664124/jobs/7033415868

I noticed the two failing tag processor tests were different to what's in Core. Assumedly something changed in the implementation during the process of committing. To fix, I simply replaced the implementation of these failing tests with the one that's in Core.

@noisysocks noisysocks added the [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. label Feb 3, 2023
Copy link
Contributor

@andrewserong andrewserong left a comment

Choose a reason for hiding this comment

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

This change LGTM since it'll be testing against the core version... however, should we also update this line: https://github.com/WordPress/gutenberg/blob/trunk/lib/experimental/html/class-wp-html-tag-processor.php#L602 to use the _doing_it_wrong line from core, for consistency? (Without this change, PHP tests might fail locally for folks if they're testing against WP 6.1)

			_doing_it_wrong(
				__METHOD__,
				__( 'Too many bookmarks: cannot create any more.' ),
				'6.2.0'
			);
			return false;

In a follow-up it'd be good to switch the GB tests over to a Gutenberg prefixed or suffixed version of the tag class, if we can.

Copy link
Contributor

@andrewserong andrewserong 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 updating! Just confirmed that the PHP tests are passing locally, and smoke tested the tag processor is outputting classnames correctly on the site frontend for the position and layout supports.

Assuming the Github Actions still pass, this LGTM, thanks for the quick PR! ✨

@noisysocks
Copy link
Member Author

noisysocks commented Feb 3, 2023

Ah yeah good point. Done.

I'll leave it up to someone else (@dmsnell, probably) to work out what to do with the WP_HTML_Tag_Processor that's in Gutenberg now that there's on in Core1 Just focused on a minimal change here to get tests passing for now.

Footnotes

  1. Gutenberg_HTML_Tag_Processor makes sense if we're planning on adding lots of new experimental features to the tag processor. If not then probably best to just make future updates directly in Core.

@github-actions
Copy link

github-actions bot commented Feb 3, 2023

Flaky tests detected in ee45b79.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4080944147
📝 Reported issues:

@noisysocks noisysocks enabled auto-merge (squash) February 3, 2023 04:17
@noisysocks noisysocks merged commit e88baef into trunk Feb 3, 2023
@noisysocks noisysocks deleted the fix/tag-processor-unit-tests branch February 3, 2023 04:38
@github-actions github-actions bot added this to the Gutenberg 15.2 milestone Feb 3, 2023
ntsekouras pushed a commit that referenced this pull request Feb 3, 2023
)

* Copy failing WP_HTML_Tag_Processor_Bookmark_Test tests from Core

* Update set_bookmark() to match Core as well
ntsekouras pushed a commit that referenced this pull request Feb 3, 2023
)

* Copy failing WP_HTML_Tag_Processor_Bookmark_Test tests from Core

* Update set_bookmark() to match Core as well
@dmsnell
Copy link
Member

dmsnell commented Feb 3, 2023

Thanks all. The current plan is simply to move the the Core version of the tag processor into lib/compact and then remove the unit tests. I'll be working on this over the next few days.

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants