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

Tag Processor: Merge independent tests into single file #45762

Merged
merged 1 commit into from
Nov 17, 2022

Conversation

dmsnell
Copy link
Member

@dmsnell dmsnell commented Nov 15, 2022

What?

Replaces #44593

Merges the PHP unit test files for the Tag Processor into a single file. In #44593 things got tricky in the merge and this patch is a recreation of the same intent.

The Tag Processor unit tests started as isolated and fast unit tests which led to following PHPUnit standards and an independence from WordPress itself. Once we adopted the WordPress standard we had to adjust the tests to fall out of line with the PHPUnit standards and we had to start integrating the tests with WordPress core to handle things like attribute escaping.

There should be no new or missing tests in this patch, but all tests which were formerly in separate files and places should now be in one file.

Why?

We made a mistake and at some point introduced four test files and now we need to bring that back to a single file, or at least get rid of the files we don't want.

How?

Merging files by comparing what's in one and not another.

Testing?

This patch changes test code. We should verify that we're not removing a test that isn't in the new single test file. We should also ensure that the tests pass. We should also ensure that no new tests are introduced during this merge.

@codesandbox
Copy link

codesandbox bot commented Nov 15, 2022

CodeSandbox logoCodeSandbox logo  Open in CodeSandbox Web Editor | VS Code | VS Code Insiders

Comment on lines -22 to -23
* Runs tests in isolated PHP process for verifying behaviors
* that depend on the `WP_DEBUG` constant value, if set.
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we keep these @dmsnell ?

@adamziel
Copy link
Contributor

@dmsnell I compared this to the updated branch I have locally and they're mostly the same, only you took a much shorter route to get there which I love. I only have one note – see above.

@dmsnell dmsnell marked this pull request as ready for review November 17, 2022 00:10
@dmsnell
Copy link
Member Author

dmsnell commented Nov 17, 2022

@adamziel I added those isolated tests in but then removed them because everything I tried to get the tests to run in CI failed. if someone can find a way to undefine WP_DEBUG then we can fix this.

I tried using the @runInSeparateProcess and @preserveGlobalState disabled special comments, tried using a different $GLOBALS['TEST_WP_DEBUG'], tried using $global $TEST_WP_DEBUG, and a few other things but every single one failed for a similar reason where the constant bled into the other tests.

the way we had it, which worked fine on our laptops, failed in CI because it was trying to serialize the exception, or the data providers, or both. by adding @preserveGlobalState disabled I got around that, but then the tests without WP_DEBUG defined were failing because WP_DEBUG was still defined from the other tests.

:sigh:

@dmsnell dmsnell merged commit dbb9487 into trunk Nov 17, 2022
@dmsnell dmsnell deleted the tag-processor/merge-tests-2 branch November 17, 2022 02:47
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