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: Add tests for CoreHeading block #10

Merged
merged 14 commits into from
Sep 26, 2024
Merged

tests: Add tests for CoreHeading block #10

merged 14 commits into from
Sep 26, 2024

Conversation

justlevine
Copy link
Collaborator

@justlevine justlevine commented Sep 14, 2024

Tracking wpengine#296

What

This PR backfills tests for the CoreHeading block and its attributes.

Tested attributes:

  • align
  • anchor
  • backgroundColor
  • className
  • content
  • cssClassName
  • fontSize
  • gradient
  • level
  • style
  • textAlign
  • textColor
  • lock

Untested fields:

  • CoreHeadingAttributes.metadata - @todo
  • CoreHeadingAttributes.placeholder - Not explicitly tested, always returns null in current tests

Exposed issues:

  • CoreHeading.cssClassNames: returns null even when the block has CSS classes. This is commented out in the test file.
  • CoreHeadingAttributes.level returns a Float instead of an Integer. This is noted in a comment in the test file.

These issues are noted in comments within the test file and should be addressed in future updates.

Copy link
Collaborator Author

@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.

@ashutoshgautams see the changes in 32cc02a:

Tl;dr

  1. Instead of setting the same block for each test, we set the post_content inside each test.
  2. Our $this->query() is shared and covers all the possible fields (generic on the query, block-specific on the fragment). We should test them all.
  3. If we think there's a bug, leave the assert, but comment it out or leave a comment nearby.
  4. Additional tests are used for different post_content to get a different set of values out of the attributes. Currently we need to test align, anchor, backgroundColor, fonts, gradient, textColor etc. (Again it's possible some will be null when they shouldnt)

Next Steps

Add tests for additional snippets to cover the attributes that are null in the current test.

(Use the block test data for inspiration: https://github.com/WordPress/theme-test-data/blob/master/64-block-test-data.xml )

@justlevine
Copy link
Collaborator Author

@ashutoshgautams I did a rebase onto the latest wpengine:main. Please make sure to do a git pull --force before committing any new changes.

- cover the attributes that are null
@ashutoshgautams
Copy link
Collaborator

ashutoshgautams commented Sep 18, 2024

Update CoreHeadingTest with Comprehensive Attribute Testing

Description

This PR updates the CoreHeadingTest class to provide more comprehensive testing of the CoreHeading block attributes in the wp-graphql-content-blocks plugin. The changes address previously null attributes and ensure that all possible configurations of the CoreHeading block are properly tested.

Changes Made

  1. Updated existing test methods to correctly check for nested style attributes.
  2. Added new test methods to cover previously untested attributes:
  • Background color
  • Text color
  • Font size
  • Custom class name
  1. Improved assertions for style-related attributes that are stored as JSON strings.

Types of Changes

  1. Bug fix (non-breaking change which fixes an issue)
  2. New feature (non-breaking change which adds functionality)
  3. Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation (if applicable).
  • My changes generate no new warnings.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.

Additional Notes

This update significantly improves our test coverage for the CoreHeading block. It should help catch any potential issues with attribute parsing or returning in future updates to the plugin.

@justlevine
Copy link
Collaborator Author

Update CoreHeadingTest with Comprehensive Attribute Testing

[... ]

This whole thing reads like GPT - and more importantly I'm pretty sure it's inaccurate.

I've updated the PR description with a template. Please edit it and fill in the things I marked.

@justlevine justlevine changed the title feat: Add CoreHeadingTest tests: Add tests for CoreHeading block Sep 18, 2024
Copy link
Collaborator Author

@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.

@ashutoshgautams - looking good 🙌

  1. Update new tests so assertEquals() tests the entire attributes array. Leave an inline comment by any attributes that are // Previously untested. by any of the earlier tests in the class.
  2. Update the PR description.
  3. Add + commit a changeset:
    1. npm run changeset
    2. Since we're just adding tests, not any new features, set the release type as patch
    3. For the message use the new PR title: "tests: Add tests for CoreHeading block".
    4. You'll see a generated json file with the info you inserted into the cli - confirm it's good before committing.

Copy link
Collaborator Author

@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.

Fix the remaining annotations and phpcbf and we'll be ready to open it on wpe

@justlevine
Copy link
Collaborator Author

justlevine commented Sep 19, 2024

(resolving all the old comments, since they're just noise in light of #10 (comment) )

phpcs.xml Outdated Show resolved Hide resolved
Comment on lines 444 to 488
public function test_retrieve_core_heading_with_background_color() {
$block_content = '
<!-- wp:heading {"backgroundColor":"vivid-red-background-color"} -->
<h2 class="wp-block-heading has-vivid-red-background-color has-background">Heading with Background Color</h2>
<!-- /wp:heading>
';

wp_update_post(
[
'ID' => $this->post_id,
'post_content' => $block_content,
]
);

$actual = graphql(
[
'query' => $this->query(),
'variables' => [ 'id' => $this->post_id ],
]
);

$block = $actual['data']['post']['editorBlocks'][0];
$attributes = $block['attributes'];

$this->assertEquals(
[
'align' => null,
'anchor' => null,
'backgroundColor' => 'vivid-red-background-color',
'className' => null,
'content' => 'Heading with Background Color',
'cssClassName' => 'wp-block-heading has-vivid-red-background-color has-background',
'fontFamily' => null,
'fontSize' => null,
'gradient' => null,
'level' => 2.0,
'lock' => null,
'placeholder' => null,
'style' => null,
'textAlign' => null,
'textColor' => null,
],
$attributes
);
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is being tested in test_retrieve_core_heading_with_font_family_and_size

Copy link
Collaborator Author

@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.

@ashutoshgautams based on the duplications, I think it might be beneficial to add doc-blocks listing the specific attributes targeted by the test. See #24 for an example of what I mean.

Add those, remove the unnecessary duplicate tests I flagged, and update the PR description, and this should be good to go.

Edit: I also think we're missing a lock test

@@ -74,6 +90,11 @@ className
';
}

/**
* Test the retrieval of core/heading block attributes.
*
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The important part of the doc blocks I was referencing was the explicit list of "previously untested" attributes, so it's clear at a glance what each test is covering.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Apart from line 94, everywhere else 'doc blocks' already contains => explicit list of "previously untested" attributes.
Added it here as well.

Copy link
Collaborator Author

@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.

@ashutoshgautams - LGTM 🙌

  1. Update the PR description to include lock
  2. (Optional) Squash the commit history into a single commit.
  3. Open a PR upstream and copy the pr title + description from here.
  4. Tag me in a comment on that new PR.

@ashutoshgautams ashutoshgautams added the has-upstream-pr A PR has been opened against wpengine's repo label Sep 24, 2024
@justlevine justlevine merged commit 8ac2f9f into main Sep 26, 2024
10 checks passed
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