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 some working examples #1

Merged
merged 16 commits into from
Apr 6, 2019
Merged

Add some working examples #1

merged 16 commits into from
Apr 6, 2019

Conversation

craigtaub
Copy link
Contributor

@craigtaub craigtaub commented Mar 14, 2019

Description of the Change

Added working examples for a couple of common configurations. More to come.

This includes

  • programmatic usage
  • react, webpack
  • basic typescript
  • basic babel
  • third party reporter

Decided to include .gitignore so we can expand the examples where necessary.

I will start adding links to Mocha's docs to link here.

Alternate Designs

More complex examples? - i decided it was best to start simple and focus on the setup, easier to expand after that.

Benefits

  • help contributors quickly setup scenario to debug (quick way to confirm a bug),
  • help users have up-to-date examples of multiple given setups
  • can be used to demonstrate common and complex functionality

@craigtaub craigtaub self-assigned this Mar 14, 2019
reporter: 'list'
});

// specify a non-default directory.

Choose a reason for hiding this comment

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

// Use non-default Mocha test directory.

@@ -0,0 +1,26 @@
var Mocha = require('mocha');

Choose a reason for hiding this comment

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

Move this line (Mocha) after the other two requires.

var mocha = require('mocha');
module.exports = MyReporter;

function MyReporter(runner) {

Choose a reason for hiding this comment

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

function MyReporter(runner, options) {

@@ -0,0 +1,23 @@
// my-reporter.js
var mocha = require('mocha');

Choose a reason for hiding this comment

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

var Mocha = require('mocha');
var Base = Mocha.reporters.Base;

module.exports = MyReporter;

function MyReporter(runner) {
mocha.reporters.Base.call(this, runner);

Choose a reason for hiding this comment

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

  Base.call(this, runner, options);

function MyReporter(runner) {
mocha.reporters.Base.call(this, runner);
var passes = 0;
var failures = 0;

Choose a reason for hiding this comment

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

Uh, why?

});

runner.on('end', function() {
console.log('end: %d/%d', passes, passes + failures);

Choose a reason for hiding this comment

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

    console.log('end: %d/%d', runner.stats.passes, runner.stats.tests);

reporter: 'spec',
slow: 75,
timeout: 2000,
spec: 'tests/**/*.spec.js',

Choose a reason for hiding this comment

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

Again with the nonstd test directory?

Copy link
Contributor Author

@craigtaub craigtaub Mar 15, 2019

Choose a reason for hiding this comment

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

Think trying to demonstrate too many things at once. Will remove and keep it simple.


- [babel](https://babeljs.io/) - transpilation
- [enzyme](https://airbnb.io/enzyme/) - React component testing library
- [chai](https://www.chaijs.com/api/assert/) - assertion library

Choose a reason for hiding this comment

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

Consider adding JSX src and test files to the mix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wasnt sure what you meant here, added a link for jsx, let me know if thats not right.

Copy link

@plroebuck plroebuck Mar 19, 2019

Choose a reason for hiding this comment

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

Not sure what was unclear -- create two JSX files, one src and one test.
Make the example use them.

Copy link
Contributor Author

@craigtaub craigtaub Mar 20, 2019

Choose a reason for hiding this comment

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

Sorry im still confused. I have jsx in both my src files, and a small bit of jsx in my test.
Could you explain a bit more please? It might help to explain the purpose of it? I'm just covering a basic component and it's text currently. Do you want it to do more?

Choose a reason for hiding this comment

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

I contributed to that confusion, I'm sure (no JSX experience). Rephrased, files containing JSX content should have ".jsx" extension. Having done recent user troubleshooting on something similar, would like example to have both "pure" JS file as well as JSX file. This will need Mocha's "extension" option to be ["js", "jsx"], I believe. Nice if we could also have "test/**/*.spec.js[x]" (again, one of each).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAIK the Facebook team itself is keen that js should be the only extension no matter what is in the file facebook/create-react-app#87 (comment). I have not seen a jsx extension in a long while personally.
I feel if we are aiming for best practices we should keep away from a .jsx extension.
Thoughts?


describe('Third party reporter usage suite', () => {
it('should equal true', () => {
equal(true, true);

Choose a reason for hiding this comment

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

Consider creating "test/unit/" and putting this there.
Create "test/e2e/" and create a spec that actually runs the reporter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have had a go at this. If you can see a cleaner way to do pls let me know.

"description": "Typescript example",
"scripts": {
"test": "mocha",
"compile": "tsc"

Choose a reason for hiding this comment

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

    "lint": "tslint --project tsconfig.json",

"description": "Typescript example",
"scripts": {
"test": "mocha",
"compile": "tsc"

Choose a reason for hiding this comment

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

Order scripts lexically

"@types/mocha": "^5.2.6",
"assert": "^1.4.1",
"mocha": "^6.0.0",
"ts-node": "^8.0.2"

Choose a reason for hiding this comment

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

    "tslint": "^5.14.0"

@craigtaub
Copy link
Contributor Author

craigtaub commented Mar 24, 2019

Thanks @juergba. I will try but might include it in the next batch of examples if thats ok.
Reason being that I think its worth drawing a line on how big this PR is getting. Also means can start sending the Repo out earlier. I definitely want to add more examples.

@craigtaub
Copy link
Contributor Author

@plroebuck think ive fixed the issues you found. Could you have another look please? Thanks

"lib": "./src",
"test": "./test"
},
"dependencies": {

Choose a reason for hiding this comment

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

Should all of these packages be devDependencies?
After transpilation, are any of them needed to load/use the package?

babel/README.md Outdated
@@ -0,0 +1,8 @@
# Babel application

Full documentation about it [here](https://mochajs.org/#-require-module-r-module)
Copy link

@plroebuck plroebuck Mar 25, 2019

Choose a reason for hiding this comment

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

Link has nothing to do the application, but that Mocha will need
-r @babel/register to test...

Copy link
Contributor Author

@craigtaub craigtaub Mar 28, 2019

Choose a reason for hiding this comment

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

I feel it's relevant and useful to point the user to the corresponding part of our documentation.

If somebody is interested to know more about a particular detail/setup in a given example the docs offer that.

Have updated the text but kept the link.

"node": ">=10.0.0"
},
"license": "ISC",
"devDependencies": {

Choose a reason for hiding this comment

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

Move "devDependencies" directly after "dependencies"


Full documentation about it [here](https://github.com/mochajs/mocha/wiki/Third-party-reporters).

To use a reporter published on npm (e.g lcov-reporter), install it locally and set `--reporter lcov-reporter`.

Choose a reason for hiding this comment

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

(e.g., lcov-reporter)

@@ -0,0 +1,10 @@
const { equal } = require("assert");

Choose a reason for hiding this comment

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

const { equal } = require('assert');

Shouldn't this file be named "my-reporter.spec.js"?

"types": "lib/index.d.ts",
"scripts": {
"compile": "tsc --declaration",
"lint": "tslint --project tsconfig.json",

Choose a reason for hiding this comment

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

prepublish?

"test": "./src"
},
"dependencies": {
"typescript": "^3.3.3"

Choose a reason for hiding this comment

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

Project still Typescript-dependent after compile (prepublish)?
If not, devDependency...

babel/.babelrc Outdated
@@ -0,0 +1,3 @@
{
"presets": ["@babel/preset-env"]
}

Choose a reason for hiding this comment

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

Should this file be replaced with "babel.config.js"? Untested content below...

module.exports = () => {
  const presets = [
      [
         "@babel/preset-env", 
         { 
             useBuiltIns: false
         }
      ]
   ];

   return { presets };
};

Copy link
Contributor Author

@craigtaub craigtaub Mar 28, 2019

Choose a reason for hiding this comment

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

Yes I think so. Done.

babel/README.md Outdated
@@ -0,0 +1,8 @@
# Babel application

Full documentation about it [here](https://mochajs.org/#-require-module-r-module)

Choose a reason for hiding this comment

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

Might want another section here. I'm no Babel expert so feel free to modify accordingly...

## Runtime Environment:

Three possibilities:
1. Avoid features which require runtime support (including new Built-ins and generators)
2. Require `@babel/runtime` directly, allowing usage of generators, but not polyfills which would modify the global environment
3. Require `@babel/polyfill` as a peer dependency and document that the consuming app must require it

This example app does not use features which require runtime support.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks good. Will add.

@@ -0,0 +1,3 @@
export const add = (a, b) => {
return a + b;
}
Copy link

@plroebuck plroebuck Mar 25, 2019

Choose a reason for hiding this comment

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

Should this be named something more descriptive (e.g., "add.js") with an actual "index.js" that exports it? Your call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point.

@craigtaub
Copy link
Contributor Author

@plroebuck think ive covered the last batch of comments. I'd like to get this in soon.

@craigtaub
Copy link
Contributor Author

Think I will merge this in a few days if there is nothing else outstanding from anyone else.

@craigtaub craigtaub merged commit 63e308a into master Apr 6, 2019
@craigtaub craigtaub deleted the addExamples branch April 6, 2019 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants