Skip to content

Background Block Support: Fix unit test HTML escaping after core change #72233

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.

Sign up for GitHub

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

Jump to bottom
Closed
talldan wants to merge 5 commits into from
Closed

Background Block Support: Fix unit test HTML escaping after core change #72233

talldan wants to merge 5 commits into from

Conversation

@talldan
Copy link
Contributor

@talldan talldan commented Oct 10, 2025 •
edited
Loading

What?

A core change (WordPress/wordpress-develop#10143) seems to have resulted in some PHP unit tests failing. This PR updates the tests in a similar way to the changes in the core PR. (I think the tests might actually be identical)

@talldan talldan self-assigned this Oct 10, 2025
@talldan talldan added [Type] Task Issues or PRs that have been broken down into an individual action to take [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. labels Oct 10, 2025
@github-actions
Copy link

github-actions bot commented Oct 10, 2025 •
edited
Loading

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: talldan <[email protected]>
Co-authored-by: ntsekouras <[email protected]>
Co-authored-by: sirreal <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@talldan talldan added Groundskeeping Maintenance No Core Sync Required Indicates that any changes do not need to be synced to WordPress Core and removed [Type] Task Issues or PRs that have been broken down into an individual action to take labels Oct 10, 2025
@talldan
Copy link
Contributor Author

talldan commented Oct 10, 2025

Ok, I'm not sure what's going on here. The tests are failing in #72231 with one output and were failing here with a different output. Not sure if it's possible that the WordPress versions in CI are different?

@talldan talldan mentioned this pull request Oct 10, 2025
Merged
@talldan
Revert "Huh?"
f600b47 
This reverts commit f9274af.
ntsekouras
ntsekouras reviewed Oct 10, 2025
View reviewed changes
Copy link
Contributor

@ntsekouras ntsekouras left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I was just looking at this and saw you beat me to it, thanks!

The job failing seems to test previous WP version, so maybe we need to have a fallback in the tests?

I'd expect us to have encountered this before and will try to search a bit.

@talldan
Copy link
Contributor Author

talldan commented Oct 10, 2025

Thanks Nik, feel free to push commits if you can figure it out.

I left a message in slack here as well - https://wordpress.slack.com/archives/C02QB2JS7/p1760082325105039.

I'm considering skipping the tests for now.

sirreal

This comment was marked as outdated.

Sign in to view

@sirreal
Copy link
Member

sirreal commented Oct 10, 2025

Does this have assertEqualHTML from Core available? That would make it resilient to this kind of change.

@talldan
Copy link
Contributor Author

talldan commented Oct 10, 2025

Lets try it! I haven't been able to run the tests locally since updating wp-env, so I'll push and hope 🤞

@ntsekouras
Copy link
Contributor

ntsekouras commented Oct 10, 2025

@talldan do you want me to wait for your CI results before pushing the change to check WP version?

@talldan
Copy link
Contributor Author

talldan commented Oct 10, 2025

It failed with a message saying it couldn't create a parser.

@ntsekouras Which WP version code are you referring to? I guess you can push the change, I don't have any better ideas.

@ntsekouras
Copy link
Contributor

ntsekouras commented Oct 10, 2025

I opened a new PR to test my changes without interfering with your CI.

@talldan
Copy link
Contributor Author

talldan commented Oct 10, 2025

It doesn't seem to have worked unfortunately, the tests fail with the opposite of whatever the expected output is 🤔

@ntsekouras
Copy link
Contributor

ntsekouras commented Oct 10, 2025

It doesn't seem to have worked unfortunately, the tests fail with the opposite of whatever the expected output is 🤔

I pushed another try. get_bloginfo might not be available when it was called in the data provider.

@talldan
Copy link
Contributor Author

talldan commented Oct 10, 2025 •
edited
Loading

Ah, I see what you mean now, there are 'WP previous major version' jobs on CI. Failures and your fix makes sense now, I'll close this in favor of your PR.

@talldan talldan closed this Oct 10, 2025
@talldan talldan deleted the fix/attachment-unit-tests branch October 10, 2025 08:31
@ntsekouras
Copy link
Contributor

ntsekouras commented Oct 10, 2025

Ah, I see what you mean now, there are 'WP previous major version' jobs on CI. Failures and your fix makes sense now, I'll close this in favor of your PR.

Still doesn't work though :). I'll try some more approaches - maybe check for a function that was introduced in 6.9..

@sirreal
Copy link
Member

sirreal commented Oct 12, 2025

I think assertEqualHTML would have worked but the arguments were wrong. The test commit needed to add <body> as the third argument before the error message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ntsekouras ntsekouras ntsekouras left review comments

@sirreal sirreal sirreal approved these changes

Assignees

@talldan talldan

Labels

Groundskeeping Maintenance No Core Sync Required Indicates that any changes do not need to be synced to WordPress Core [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@talldan @sirreal @ntsekouras