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: backfill tests for ContentBlockResolver #12

Merged
merged 9 commits into from
Sep 26, 2024

Conversation

Ta5r
Copy link
Collaborator

@Ta5r Ta5r commented Sep 16, 2024

Tracking: wpengine#292 (comment)

What

This PR backfills additional test cases for the ContentBlocksResolver class in ContentBlocksResolverTest.

How

The following test cases are included:

  • Pre-resolve Filter Test: test_pre_resolved_blocks_filter_returns_non_null ensures that the wpgraphql_content_blocks_pre_resolve_blocks filter correctly modifies the block content before the blocks are resolved.
  • Empty Content Test: test_returns_empty_array_for_empty_content checks that an empty array is returned when the post content is empty, ensuring proper behavior for posts with no blocks.
  • Post-resolve Filter Test: test_filters_wpgraphql_content_blocks_resolve_blocks ensures that the wpgraphql_content_blocks_resolve_blocks filter correctly modifies block content after the blocks are resolved.
  • Inner Block Test: test_inner_blocks tests resolution for both flat and nested blocks, including the correct parentClientIds are set, and that there are no discrepancies between flat/nested resolution.

@Ta5r Ta5r self-assigned this Sep 16, 2024
@justlevine justlevine changed the base branch from main to dev/refactor-ContentBlocksResolver September 16, 2024 20:29
@justlevine justlevine force-pushed the dev/refactor-ContentBlocksResolver branch from f0860a6 to 31944d8 Compare September 16, 2024 21:04
@justlevine justlevine force-pushed the test/refactor-ContentBlockResolver branch from ad75095 to 09f83cc Compare September 17, 2024 14:52
Copy link
Collaborator

@justlevine justlevine left a comment

Choose a reason for hiding this comment

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

@Ta5r -

I rebased this PR on the latest dev/refactor-ContentBlocksResolver, so remember to git pul --force before committing any new changes.

tests/unit/ContentBlocksResolverTest.php Outdated Show resolved Hide resolved
tests/unit/ContentBlocksResolverTest.php Outdated Show resolved Hide resolved
tests/unit/ContentBlocksResolverTest.php Outdated Show resolved Hide resolved
tests/unit/ContentBlocksResolverTest.php Outdated Show resolved Hide resolved
tests/unit/ContentBlocksResolverTest.php Show resolved Hide resolved
tests/unit/ContentBlocksResolverTest.php Outdated Show resolved Hide resolved
tests/unit/ContentBlocksResolverTest.php Show resolved Hide resolved
@justlevine justlevine changed the title Updating tests for refactored content block resolver ( in progress 🏗️ ) tests: backfill tests for ContentBlockResolver (post-refactor) Sep 17, 2024
Base automatically changed from dev/refactor-ContentBlocksResolver to main September 19, 2024 10:19
@justlevine justlevine force-pushed the test/refactor-ContentBlockResolver branch from 844138a to fcde1f2 Compare September 19, 2024 13:39
@justlevine justlevine changed the title tests: backfill tests for ContentBlockResolver (post-refactor) tests: backfill tests for ContentBlockResolver Sep 21, 2024
@justlevine justlevine marked this pull request as ready for review September 21, 2024 17:41
Copy link
Collaborator

@justlevine justlevine left a comment

Choose a reason for hiding this comment

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

@Ta5r Take a look at c5f0410 where:

  1. I removed tests that were already being covered (per the previous comments)
  2. I refactored the inner blocks test so it's more clear what exactly it is asserting (general rule: avoid transforming results, instead create an ::assert*() that holds the relevant transformation logic.

Also cleaned up the PR description (they have CI so no need for a screenshot, and removed the jargon so it's more clearly indicates what's included).

Go ahead and generate a changeset, and open up a PR upstream.

@justlevine justlevine added the has-upstream-pr A PR has been opened against wpengine's repo label Sep 23, 2024
@justlevine justlevine merged commit b709913 into main Sep 26, 2024
10 checks passed
@justlevine justlevine deleted the test/refactor-ContentBlockResolver branch October 18, 2024 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has-upstream-pr A PR has been opened against wpengine's repo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants