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

fix(scripts): rename script from test-unit to test-unit-js #86

Merged
merged 1 commit into from
Feb 21, 2018

Conversation

ntwb
Copy link
Member

@ntwb ntwb commented Feb 21, 2018

Follow up to #83

This PR seeks to update and change the default script name from test-unit to test-unit-js

The goal behind this to facilitate what was discussed in #83 (comment) where a configuration for WordPress Core can use test-unit as a catch all task for not just JavaScript tasks, e.g:

Originally a configuration of was suggested:

{
    "scripts": {
        "test-unit:js:qunit": "wp-scripts test-unit-qunit",
        "test-unit:js:jest": "wp-scripts test-unit-jest",
        "test-unit:js": "npm-run-all test-unit:js:*",
        "test-unit:php": "wp-scripts test-unit-phunit",
        "test": "npm-run-all test-unit:**",
    },
}

The proposed solution in this PR would then allow for more semantic naming of the tasks that could then facilitate the following flexible configurations:

Cfg 1: This option is quite verbose in listing each of the available test-unit-* scripts available, it then uses npm-run-all scripts to group the scripts by language, and finally another npm-run-all script to group those together.

{
    "scripts": {
        "test-unit:js:qunit": "wp-scripts test-unit-qunit",
        "test-unit:js:jest": "wp-scripts test-unit-jest",
        "test-unit:js": "npm-run-all test-unit:js:*",
        "test-unit:php:wordhat": "wp-scripts test-unit-wordhat",
        "test-unit:php:phpunit": "wp-scripts test-unit-phpunit",
        "test-unit:php": "npm-run-all test-unit:php:*",
        "test": "npm-run-all test-unit:**",
    },
}

Cfg 2: This configuration is nowhere near as verbose as Cfg 1, a couple of test-unit-* language scripts then grouped together by a single npm-run-all task.

{
    "scripts": {
        "test-unit:js": "wp-scripts test-unit-js",
        "test-unit:php": "wp-scripts test-unit-php",
        "test": "npm-run-all test-unit:*",
    },
}

Cfg 3: Lastly, the simplest of each config option, a single stand-alone test-unit task.

{
    "scripts": {
        "test": "wp-scripts test-unit",
    },
}

Each of the above script names above includes semantic meaning in their naming when reading the script name each name clearly describes itself.

Each of these script names clearly describes itself
test-unit-qunit
test-unit-jest
test-unit-wordhat
test-unit-phpunit
test-unit-js
test-unit-php

Without this PR the script name test-unit is quite generic, there is no way to know without looking at the code that this is a JavaScript only script, in fact not even that, its an alias of test-unit-jest

Switching test-unit to test-unit-js allows us to use test-unit as a higher level alias, it can then be used to call both test-unit-qunit and test-unit-jest if both Qunit and Jest tests are detected in the target repo, likewise PHP tasks, if PHPUnit tests are detected it can also include those.

@@ -30,11 +30,11 @@ This is how you execute those scripts using the presented setup:

## Available Scripts

### `wp-scripts test-unit`
### `wp-scripts test-unit-js`
Copy link
Member

Choose a reason for hiding this comment

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

I think we should swap the title and alias :)

@gziolo
Copy link
Member

gziolo commented Feb 21, 2018

I'm wondering if you can use npm-run-all as part of the node script, too. If it would work or there is an alternative we could definitely support wp-scripts test-unit having all the underlying scripts executed. At the same time, everyone could pick the tool they need with the granular naming scheme.

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Code LGTM, thanks for tackling the follow-up PR 👍

I'm wondering if we should put the summary of what you described in this PR in a README file that would live in the repository, so we could refer to it when discussing further PRs. What do you think?

@gziolo gziolo merged commit 9944a02 into master Feb 21, 2018
@gziolo gziolo deleted the update/use-test-unit-js branch February 21, 2018 10:26
@aduth
Copy link
Member

aduth commented Feb 21, 2018

Can someone explain why we use - and : interchangeably seemingly at random?

Why not test:unit instead of test-unit ?

Why not in these example configurations test-unit-js instead of test-unit:js ?

Can't we just pick one and stick with it? Conventionally I've seen this as : for variations of the "type" of script.

@ntwb
Copy link
Member Author

ntwb commented Feb 22, 2018

The 3 config options I listed above are examples of how an external repo may use the scripts in their package.json file provided by this repo

For example from cfg 2. above:

The external repo task names on the left use the : colon and are local to that repo only, each of the scripts run by those tasks is a @wordpress/packages/scripts script that uses the - hyphen:

{
    "scripts": {
        "test-unit:js": "wp-scripts test-unit-js",
        "test-unit:php": "wp-scripts test-unit-php",
        "test": "npm-run-all test-unit:*",
    },
}

The above could also be rewritten as:

{
    "scripts": {
        "test:js": "wp-scripts test-unit-js",
        "test:php": "wp-scripts test-unit-php",
        "test": "npm-run-all test:*",
    },
}

Or rewritten as:

{
    "scripts": {
        "foo": "wp-scripts test-unit-js",
        "bar": "wp-scripts test-unit-php",
        "test": "npm-run-all foo bar",
    },
}

The npm-run-all package has a somewhat widespread usage nomenclature using the : colon:

{
    "scripts": {
        "sometask:1": "abc",
        "sometask:2": "def",
        "sometask:3": "ghi",
        "sometask:4": "xyz",
        "task": "npm-run-all sometask:*",
    },
}

The result is that running in npm run task above runs all 4 of the sometask tasks

@gziolo
Copy link
Member

gziolo commented Feb 22, 2018

Yes, we use only - as a separator in this repository. We could start using : for the grouping purpose but only if we would like to include the glob like pattern matching that npm-run-all uses to execute a few scripts in a sequence (can be also in parallel with a flag).

I think we can leave it as it is and continue using -. At the moment we map task name to the file name, e.g. test-unit-jest loads scripts/test-unit-jest.js file.

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

Successfully merging this pull request may close these issues.

3 participants