Skip to content
This repository has been archived by the owner on Jul 9, 2018. It is now read-only.

Scripts: New scripts package containing test command #62

Merged
merged 3 commits into from
Jan 25, 2018

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Jan 9, 2018

This is another try since in #55 we decided that all existing plugin commands i18n and readme can be implemented using WP-CLI.

Why

Command-line interfaces like WP-CLI or create-react-app help to turn bootstrapping an app into a pleasant experience, but it is still not enough to keep it easy to maintain in the long run. Developers are left on their own to keep all configurations and dependent tools up to date. This problem multiplies when they own more than one project which shares the same setup. Fortunately, there is another pattern that can simplify maintainers life – reusable scripts. This idea boils down to moving all the necessary configurations and scripts to one single tool dependency. In most cases, it should be possible to accomplish all tasks using the default settings, but some customization is allowed, too. With all that in place updating all projects becomes a very straightforward task. Some existing examples that I found are:

You can read more about the idea behind this PR in my blog post Starter kit and reusable scripts.

How

This implementation is based on the react-scripts.

The end goal would be to run it as:

  • wp-scripts test

To include you would have to include in devDependencies only this:

"@wordpress/scripts": "1.0.0"

We should also consider providing reusable scripts that could allow to flawlessly incorporate more advanced JavaScript tooling like build step, code transpilation, linting and so on. The testing command is just an example I started with because I'm the most familiar with the Jest setup.

Testing

I'm not quite sure how to test it for external usage before this package is published. However, this package is locally used to test the entire repository using npm test command. You can also run this script from the root folder using ./packages/scripts/bin/wp-scripts.js test.

@codecov
Copy link

codecov bot commented Jan 9, 2018

Codecov Report

Merging #62 into master will decrease coverage by 4.97%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #62      +/-   ##
==========================================
- Coverage   78.52%   73.55%   -4.98%     
==========================================
  Files          21       23       +2     
  Lines         340      363      +23     
  Branches       71       76       +5     
==========================================
  Hits          267      267              
- Misses         61       79      +18     
- Partials       12       17       +5
Impacted Files Coverage Δ
packages/scripts/bin/wp-scripts.js 0% <0%> (ø)
packages/scripts/scripts/test-script.js 0% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3d50441...0e7988f. Read the comment docs.

@ahmadawais
Copy link
Contributor

@gziolo this is so on point with what I have been trying to build lately. A go to tool that takes care of all these deps and provides an easy + updateable solution with zero configuration. Would love to contribute.

@gziolo
Copy link
Member Author

gziolo commented Jan 9, 2018

@ahmadawais, you are welcome to contribute. Is there something specific you would like to tackle first?

@gziolo
Copy link
Member Author

gziolo commented Jan 21, 2018

@ntwb or @adamsilverstein, do you know if it can be unit tested somehow or is it enough that it works as expected as long as the tests for the whole repository work using wp-scripts test we are good? If yes, we should ignore codecov complaints.

I moved Babel setup from Gutenberg, but ignored i18n part which is its own story. I think we should tackle it separately when moving i18n to packages. I will open a follow-up PR where I extract Babel configuration to its own preset package.

I will also create another preset for Jest config to make sure we share the same defaults between packages and Gutenberg. At the moment wp-scripts will always use local config files, so this isn't a blocker.

@gziolo gziolo changed the title Scripts: New scripts package containing test command Scripts: New scripts package containing test command Jan 21, 2018
@gziolo gziolo force-pushed the add/scripts-jest branch 2 times, most recently from f72dda2 to 67a0aea Compare January 23, 2018 09:15
 - @wordpress/a11y@1.0.4
 - @wordpress/autop@1.0.2
 - @wordpress/babel-preset-default@1.0.1-0
 - @wordpress/dom-ready@1.0.1
 - @wordpress/hooks@1.1.2
 - @wordpress/jest-console@1.0.1
 - @wordpress/scripts@1.0.1-0
 - @wordpress/url@1.0.1
@jaswrks
Copy link

jaswrks commented Jan 23, 2018

❤️ Nice! In a way, this seems like the very beginning of WP-CLI.js.

@adamsilverstein
Copy link
Member

I'm not sure how you would add tests, you would want to run the script command itself and look for the expected output but I've never tried anything like that with the testing frameworks I have used. I'm curious though and will try to do some research.

@ahmadawais
Copy link
Contributor

@adamsilverstein I am also curious about that? Figuring out how I can add automated testing to create-guten-block that tests cgb + it's WordPress counter part.

Copy link
Member

@ntwb ntwb left a comment

Choose a reason for hiding this comment

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

I think this is great, I originally didn't grok the exact purpose of these scripts and who would benefit from them, I do now ❤️

I've suggested 1.0.0-alpha.0, if we can update that and add a couple of minor verbiage tweaks then it's ready to merge and should be published ASAP 🎉

p.s. Apologies for my delay reviewing this :)

@@ -0,0 +1,38 @@
{
"name": "@wordpress/scripts",
"version": "1.0.1-0",
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a fan of this naming schema, I'd much prefer we use "prerelease" tags, alpha, beta, rc etc

https://docs.npmjs.com/all#prerelease-tags 1.1.0-alpha.0, 1.2.4-beta.1, etc.,

How about for starters we use 1.0.0-alpha.0 here now, I'm keen to iterate on these quickly by integrating them into some of the other repos I work on, we can publish these updates frequently to npm using this npm compatible semantic version nomenclature (https://docs.npmjs.com/all#caret-ranges-123-025-004)

Copy link
Member Author

Choose a reason for hiding this comment

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

I will publish 1.0.0 soon and stick with alpha in the future. I'm still learning how to use Lerna. Thanks for a good hint :)

Copy link
Member

Choose a reason for hiding this comment

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

via https://github.com/lerna/lerna#--cd-version

"If you have any packages with a prerelease version number (e.g. 2.0.0-beta.3) and you run lerna publish with --cd-version and a non-prerelease version increment (major / minor / patch), it will publish those packages in addition to the packages that have changed since the last release."

That should work for Lerna and 1.0.0-alpha.0 by my reading

Copy link
Contributor

Choose a reason for hiding this comment

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

How about using the --canary command? That's more granular and being used by other popular projects like CRA and I am using it on CGB create-guten-block.

lerna publish --independent --npm-client=npm --canary

Copy link
Member Author

@gziolo gziolo Jan 25, 2018

Choose a reason for hiding this comment

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

We have this in npm run publish:dev:
https://github.com/WordPress/packages/blob/master/package.json#L36

"publish:dev": "npm run build-clean && npm run build && lerna publish --npm-tag next"

We might want to revisit how it's configured to simplify the process.

@@ -5,6 +5,7 @@
"author": "WordPress",
"license": "GPL-2.0-or-later",
"keywords": [
"wordpress",
Copy link
Member

Choose a reason for hiding this comment

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

Happy to ignore this unrelated change and pretend I didn't see it 😏

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, we should just add wordpress keyword to all package.json files in its own PR.


## Setup

This is a CLI and exposes a bin called `wp-scripts` so you can call it directly. However this module is designed to be configured using `scripts` section in the `package.json` file of your project. Example configuration:
Copy link
Member

Choose a reason for hiding this comment

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

A couple of minor verbiage tweaks please:
• "exposes a binary called" <- Use binary over bin
• "designed to be configured using the scripts" < Added the


### `wp-scripts test`

Launches the test runner. It uses [Jest](https://facebook.github.io/jest/) behind the scenes and you are allowed to use all its [CLI options](https://facebook.github.io/jest/docs/en/cli.html). You can also run `./node_modules/.bin/wp-scripts --help` or `npm run test:help` (if you use `package.json` setup shared above) to view all available options.
Copy link
Member

Choose a reason for hiding this comment

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

A couple of minor verbiage tweaks please:
• "...behind the scenes and you are able to utilize all of its CLI options."
• "...setup shared above) to view all of the available options."

"keywords": [
"wordpress",
"scripts",
"reusable-scripts"
Copy link
Member

Choose a reason for hiding this comment

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

Can we add wordpress-scripts please 🙏

(It would fit in nicely listed here: https://github.com/reyronald/awesome-toolkits#other)

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure thing :)

@gziolo gziolo merged commit 7ee6367 into master Jan 25, 2018
@gziolo gziolo deleted the add/scripts-jest branch January 25, 2018 11:05
@gziolo
Copy link
Member Author

gziolo commented Jan 25, 2018

@ntwb thanks your review, the last missing part is Jest preset and we can update Gutenberg repository. I will open another PR tomorrow. Unfortunately setting preset for Jest isn't as convenient as in case of Babel.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants