Skip to content
This repository has been archived by the owner on Aug 13, 2023. It is now read-only.

[DO NOT MERGE] POC tests at component level #247

Closed
wants to merge 9 commits into from

Conversation

radiocontrolled
Copy link
Contributor

@radiocontrolled radiocontrolled commented Dec 31, 2018

Resolves #193

Tests at the component level
This PR creates a psammead-test component, with Mocha and Chai installed as devDependencies in the component's package.json. The component has a test script, which runs a test (relevant doc):

  "scripts": {
    "test": "mocha --compilers js:babel-register"
  },

Another test script, which echoes to the console, is added to psammead-brand.

Testing at the parent level
In the parent package.json of Psammead, I added a "test:packages" script. It calls lerna run test which will look for a script of name 'test' in each package and will run it (see lerna run docs). I chose lerna run test over lerna exec -- npm test because it's cleaner - the later will return "Error: no test specified" for all components that lack a test script.

"scripts": {
    ...
    "test": "npm run test:lint && npm run test:lint:css && npm run test:unit && npm run test:packages",
    "test:packages": "lerna run test",
   ...
}

Reviewing this PR

  1. Install Lerna globally if it is not installed
    npm install --g lerna
    lerna --version

  2. Do npm test

The usual linting and Jest unit tests will run. This will be followed by the test scripts from psammead-brand and psammead-test.

Implications
New components can be added, with their own testing devDependencies (e.g. mocha/chai in this POC). They might be hooked into the psammead level npm test script by being called a consistent name (e.g. "test") and invoked via lerna run test.

However, mandating a consistent script name for component-level tests could prove annoying. Let's say there are several Psammead components have bespoke component-level tests, psammead-component-A, psammead-component-B and psammead-component-C. It might be annoying for the author of psammead-component-C to run test scripts from other components if this adds significantly to testing time.

  • I have assigned myself to this PR and the corresponding issues
  • Tests added for new features
  • Test engineer approval

@radiocontrolled radiocontrolled self-assigned this Dec 31, 2018
Copy link
Contributor

@dr3 dr3 left a comment

Choose a reason for hiding this comment

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

👍 We would need to document somewhere that we auto run .test.js files using our testing suite, however you can do your own testing using a test folder or .spec.js files.

As a POC looks great, make sure this doesn't get merged or @bbc/psammead-test will publish though 🙈

Copy link
Contributor

@bcmn bcmn left a comment

Choose a reason for hiding this comment

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

This looks great to me, nice & understandable. 👍

Definitely agree on Drew's point though, we'd have to document the Jest behaviour.

@dr3 dr3 changed the title POC tests at component level [DO NOT MERGE] POC tests at component level Jan 2, 2019
@radiocontrolled
Copy link
Contributor Author

radiocontrolled commented Jan 2, 2019

@dr3 @bcmn I agree. I think some of this documentation would belong in CONTRIBUTING.md (how to add custom tests) and maybe in the top level readme (explanation of current behaviour). Will solicit some more feedback on this PR.

The current Jest behaviour looks for scripts and packages that have test.js/test.jsx files. It also looks for a __tests__ folder

  "jest": {
    "collectCoverageFrom": [
      "**/packages/**/*.{js,jsx}",
      "scripts/**",
      ...
    ],
    "testMatch": [
      "**/__tests__/**/*.js?(x)",
      "**/*.test.{js,jsx}"
    ]
  },

@radiocontrolled
Copy link
Contributor Author

This PR should not be merged.
It's closed as the outcome will be a PR to contributing.

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.

3 participants