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

Add a block fixtures framework to allow automated checking of block validation #18643

Merged

Conversation

glendaviesnz
Copy link
Contributor

@glendaviesnz glendaviesnz commented Feb 1, 2021

Note Needs to be merged into #18582 before it is merged.

Adds automated checking of block validation for Jetpack blocks. See p1HpG7-b6y-p2 for background.

Based on the Gutenberg core block fixtures, but restructured to put the fixtures next to the block in question.

Only runs for WhatsApp block currently.

To generate new fixtures:

yarn fixtures:generate extensions/blocks/send-a-message/whatsapp-button/test/validate.js

To run validation tests:

yarn test-extensions-single extensions/blocks/send-a-message/whatsapp-button/test/validate.js

See the readme file for more details.

@jetpackbot
Copy link

jetpackbot commented Feb 1, 2021

Warnings
⚠️ The Privacy section is missing for this PR. Please specify whether this PR includes any changes to data or privacy.
⚠️ "Proposed changelog entry" is missing for this PR. Please include any meaningful changes
⚠️ "Testing instructions" are missing for this PR. Please add some

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against 3bb5e46

@glendaviesnz glendaviesnz changed the title [WIP]: Add a block fixtures framework to allow automated checking of block validation Add a block fixtures framework to allow automated checking of block validation Feb 1, 2021
Copy link
Contributor

@aaronrobertshaw aaronrobertshaw left a comment

Choose a reason for hiding this comment

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

This tests well for me 👍

  • All tests passed when run as per instructions
  • Altering the fixture or the save function correctly caused test failures
  • Altering the number of deprecations was also caught
  • Deleting fixture files was handled
  • Regeneration of fixtures worked and tests were passing again
  • Took it for a test run on the jetpack/rating-star block and appeared fine
  • Only one minor issue in code, inline comment left.

It would be good to get a few extra sets of eyes on this though with regards to overall approach. In the meantime, I have a couple of questions or thoughts on it.

Naming convention for "settings"

In #18582 the block's controls get refactored out into a settings.js file. Now within this PR we are importing block settings as settings within validate.js. It might be good to establish some clearer naming conventions around this now at the outset.

Automating script/fixture generation

Can we automate the creation of the initial fixture .html file at all?

It seems like the simple steps of inserting a block with blank or default attributes and running it through the save function could be doable. Perhaps we could use the block settings' example.attributes if available?

Similar question for the validate.js can we script it's initial creation?

I'm thinking making it super simple to get the files in place might help encourage inclusion of these tests for more blocks.

@glendaviesnz
Copy link
Contributor Author

Naming convention for "settings"
In #18582 the block's controls get refactored out into a settings.js file.

Good point - in #18582 I have renamed the toolbar and sidebar component to 'Configuration' to avoid any confusion

@glendaviesnz
Copy link
Contributor Author

Automating script/fixture generation
Can we automate the creation of the initial fixture .html file at all?

Similar question for the validate.js can we script it's initial creation?

Could be worth looking at. I will get some more feedback on the concept in general first and then take a look at that.

Copy link
Contributor

@creativecoder creativecoder left a comment

Choose a reason for hiding this comment

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

Have only skimmed through this, for now, but ❤️ adding this kind of test!

It'd be great to add a section to the block fixtures readme with a step by step process for updating when creating a deprecation... I imagine that's going to be "first contact" with this system for many folks working on existing blocks.

@brbrr
Copy link
Contributor

brbrr commented Feb 8, 2021

<3 it!

I left few questions inline, but I also have a few more general questions.

  • Is there a way to run all fixture validation tests with a single command? It could be useful when we would be adding it into our CI setup.
  • Do you plan to add these new tests into CI?

@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello glendaviesnz! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D56672-code works as expected before merging this PR. Once this PR is merged, please commit the changes to WP.com. Thank you!
This revision will be updated with each commit to this PR

@glendaviesnz
Copy link
Contributor Author

It'd be great to add a section to the block fixtures readme with a step by step process for updating when creating a deprecation

Have added some details, let me know if it make sense or not.

@glendaviesnz
Copy link
Contributor Author

  • Is there a way to run all fixture validation tests with a single command? It could be useful when we would be adding it into our CI setup.

The validation tests all get run as part of yarn test-extensions test script, but have added a separate script fixtures:test to allow running just the validation tests.

  • Do you plan to add these new tests into CI?

I am assuming that because they all run as part of test-extensions they will be pulled into CI by default? In gutenberg they just run as part of the jest unit test run, do you think this is appropriate in this context or should they be excluded from unit tests runs and run as a separate set of tests?

@brbrr
Copy link
Contributor

brbrr commented Feb 10, 2021

The validation tests all get run as part of yarn test-extensions test script

Oh, I didn't know that. that should take care of CI run. Happy to approve it once you'll get a green light from one of your teammates

@apeatling apeatling self-requested a review February 10, 2021 19:42
Copy link
Member

@apeatling apeatling left a comment

Choose a reason for hiding this comment

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

Great work on this! Works well for me, and happy to see this implemented in new and existing blocks going forward. 👍🏻

Copy link
Contributor

@brbrr brbrr left a comment

Choose a reason for hiding this comment

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

I guess you don't need approval from Crew to merge this branch into another (non-master) branch, but here it goes anyway :)

:shipit:

@glendaviesnz glendaviesnz force-pushed the try/jetpack-block-additional-tests branch from fab9c33 to 0090356 Compare February 14, 2021 21:29
@github-actions github-actions bot added the [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ label Feb 17, 2021
@glendaviesnz glendaviesnz merged commit 5125172 into try/jetpack-block-additional-tests Feb 17, 2021
@glendaviesnz glendaviesnz deleted the try/block-validation-tests branch February 17, 2021 02:11
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Feb 17, 2021
glendaviesnz added a commit that referenced this pull request Feb 17, 2021
…alidation (#18643)

Co-authored-by: Glen Davies <glen.davies@a8c.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Send a message [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ Touches WP.com Files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants