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 CI check for newspack-blocks textdomain #44323

Merged
merged 12 commits into from
Jul 23, 2020

Conversation

noahtallen
Copy link
Contributor

@noahtallen noahtallen commented Jul 21, 2020

cc @akirk (for textdomain)
cc @scinos (for yarn install time)
cc @Automattic/ajax (for changes to the newspack sync step)

Follows conversation from #44125 (comment).

closes #44296

Changes proposed in this Pull Request

  1. Move newspack-block synchronization to the build step rather than install step. This shaves 10 seconds off yarn install. IMO, this is preferred behavior since most wp-calypso devs do not care about it. Additionally, this should not change existing workflows, since you will have to run the build script before testing anyways.
  2. Introduce workflow for newspack-blocks. This fails if:
    a) the newspack-blocks synced files do not exist
    b) the php textdomain of any synced files is not full-site-editing
  3. Fail the build script if the build fails. This was a bug where the build script always exited with 0.
  4. Fail the newspack sync script if phpcbf fails, triggering a build failure (reported in CI.)
  5. Add a helpful message if the newspack sync script caused a build failure. This informs people that they might need to run composer install.

The convenient thing about the workflow is that it tests the generated artifact -- i.e. exactly the files which will be installed in WordPress.

Testing instructions

Locally, you can test both yarn and yarn build for the FSE plugin. However, most of this needed to be verified in CI. (see my screenshots in the comment below for proof)

  • CI should pass.
  • Test that CI fails if sync step does not happen
  • Test that CI fails if there is an issue with phpcbf
  • Test that CI fails if textdomain is not full-site-editing

@noahtallen noahtallen requested a review from a team as a code owner July 21, 2020 18:52
@noahtallen noahtallen self-assigned this Jul 21, 2020
@matticbot
Copy link
Contributor

@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jul 21, 2020
@noahtallen noahtallen removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jul 21, 2020
@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

@akirk
Copy link
Member

akirk commented Jul 21, 2020

Great idea and approach, thank you! Didn't take that much of a closer look yet but will do tomorrow.

@matticbot
Copy link
Contributor

Caution: This PR affects files in the FSE Plugin on WordPress.com
Please ensure your changes work on WordPress.com before merging.

D46698-code has been created so you can easily test it on your sandbox. See this FieldGuide page about developing in the FSE Plugin for more info: PCYsg-ly5-p2

@noahtallen
Copy link
Contributor Author

noahtallen commented Jul 21, 2020

  1. It fails when there are bad PHP textdomains in the synced newspack blocks:

Screen Shot 2020-07-21 at 12 15 52 PM

2. It fails when the newspack blocks have not been synced:

Screen Shot 2020-07-21 at 12 25 17 PM

3. Build fails when phpcbf fails:

Screen Shot 2020-07-21 at 12 56 01 PM

4. It displays a helpful error message when the newspack build fails:

Screen Shot 2020-07-21 at 12 59 24 PM

@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jul 21, 2020
@deBhal
Copy link
Contributor

deBhal commented Jul 21, 2020

I can't comment on the yarn/lerna lifecycle changes, and I made a couple of small suggestions and re-started the e2e builds to clear out an unrelated timeout, but it looks great overall 👍

@akirk
Copy link
Member

akirk commented Jul 22, 2020

Thank you, @deBhal for the assessemnt. I don't really have anything to add other than this is really helpful to avoid this in future. Thank you!

@noahtallen noahtallen force-pushed the try/better-phpcbf-in-newspack-blocks branch from 025a27c to fa2d712 Compare July 22, 2020 19:57
@noahtallen
Copy link
Contributor Author

I've addressed the comments. it might be nice to get a ✅ , but I may merge anyways by the end of the day since this shouldn't affect much.

@scinos
Copy link
Contributor

scinos commented Jul 23, 2020

Thanks for improving yarn install times for everybody :)

@noahtallen noahtallen force-pushed the try/better-phpcbf-in-newspack-blocks branch from fa2d712 to d898422 Compare July 23, 2020 21:11
@noahtallen noahtallen merged commit 5012986 into master Jul 23, 2020
@noahtallen noahtallen deleted the try/better-phpcbf-in-newspack-blocks branch July 23, 2020 21:44
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jul 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FSE plugin: Add unit test to confirm that textdomains have been re-written
5 participants