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

Tests: Improve code coverage for _build_block_template_result_from_file #5396

Closed

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Oct 4, 2023

Trac ticket: https://core.trac.wordpress.org/ticket/59325

Follow-up to the comment from @costdev in #5186 (comment):

Here's a coverage report which includes existing tests and tests included in this PR.

image
image

Everything that this PR intends to cover appears to be covered. 🙂

In terms of the remaining coverage for this function:

  • Line 585 isn't hit by the tests that specifically cover this function.
  • Two branches aren't covered.
    • One branch can be covered by testing with a postTypes key in $template_file and a $template_type of wp_template.
    • One branch can be covered by ensuring both outcomes of line 572 are met by tests, then testing $template->title for the expected result.
  • There don't appear to be tests specifically for this function that provide a non-default slug in $template_file. This means lines 578-581, while hit by the tests, don't appear to have coverage to ensure that $template->title is retained for wp_template types with a non-default slug in the $template_file.

As the uncovered parts don't seem to be part of this PR's goals, we can follow up to complete coverage for _build_block_template_result_from_file(). I'd also suggest that we start to separate the tests for each function into their own files, completing @ticket, @covers, and any other annotations that need to be added. This makes it easier not to get lost amongst the tests.

All cases listed should be covered with this PR.


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

@gziolo gziolo self-assigned this Oct 4, 2023
@gziolo gziolo requested a review from costdev October 4, 2023 08:08
@gziolo gziolo changed the title Tests: Improve code coverage for `_build_block_template_result_from_f… Tests: Improve code coverage for _build_block_template_result_from_file Oct 4, 2023
@gziolo gziolo requested a review from ockham October 4, 2023 09:07
Copy link
Contributor

@ockham ockham left a comment

Choose a reason for hiding this comment

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

Left a minor note about test method names, but nothing blocking.

Thank you! LGTM 😊

@gziolo
Copy link
Member Author

gziolo commented Oct 4, 2023

Thank you for the feedback. I’ll rename and move test cases as suggested and land tomorrow 👍

@gziolo gziolo force-pushed the update/build-block-template-tests branch 2 times, most recently from 83919a5 to c656135 Compare October 11, 2023 12:46
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 updates @gziolo! This looks good to me!

Coverage shows the hooked_blocks() lines aren't covered by these tests.

It would be good to cover these two conditions so there's a complete set of tests in this function's test class. Even if covered by tests for another function, having tests here means that if these tests pass and the other function's fail, we know to look in the other function right away.

image

That said, the final coverage doesn't need to be done in this PR and could be a follow-up if you'd like.

Approving 🙂

@gziolo
Copy link
Member Author

gziolo commented Oct 12, 2023

It would be good to cover these two conditions so there's a complete set of tests in this function's test class.

Do I understand correctly that you mean adding coverage in the same test suite? I'm sure it has coverage in other places, but I agree we can add another test here 👍🏻

I want to wait until #5455 as I will have to rebase this branch and probably remove one or two tests, maybe move some new tests. I can work on the feedback as part of that.

@gziolo gziolo force-pushed the update/build-block-template-tests branch from f0250e3 to fecdb3b Compare October 23, 2023 04:48
@gziolo
Copy link
Member Author

gziolo commented Oct 23, 2023

Committed with https://core.trac.wordpress.org/changeset/56983.

It would be good to cover these two conditions so there's a complete set of tests in this function's test class.

I will work on more tests separately covering the filter hooked_block_types, and I will tackle it as part of that effort.

@gziolo gziolo closed this Oct 23, 2023
@gziolo gziolo deleted the update/build-block-template-tests branch October 23, 2023 05:40
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.

3 participants