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

Unit test #40

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

Unit test #40

wants to merge 18 commits into from

Conversation

Solinca
Copy link

@Solinca Solinca commented Aug 5, 2018

Implementation of unit tests ( mocha + chai + istanbul )
Covering 2 files :
SwfObjectProcessor/addClassNames.js
SwfObjectProcessor/createSymbols.js

Copy link

@jperdereau jperdereau left a comment

Choose a reason for hiding this comment

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

Hey @Solinca thx for the contribution
here some small advices

package.json Outdated
@@ -10,17 +10,26 @@
"bin": {
"jeff": "./bin/jeff"
},
"scripts": {
"test": "NODE_ENV=test mocha './**/*.test.js'",
"istanbul": "istanbul cover _mocha $(find ./ -name \"*.test.js\" -not -path \"./node_modules/*\")"

Choose a reason for hiding this comment

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

Usually we are putting all the test files inside a test/ folder

in that case you can simplify the npm scripts:

  • mocha should look into the test folder and should be recursive
  • istanbul do not have to find all the test files

Copy link
Author

@Solinca Solinca Aug 7, 2018

Choose a reason for hiding this comment

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

Oh right!
I already put all my tests into an "unit-tests" folder.
My unit test setup file was located on root before I removed it, this is the reason why I wrote those line.
I will apply these changes

it('add two or more class name', function () {
const min = 2;
const max = 5;
const random = Math.floor(Math.random() * (max - min + 1)) + min;

Choose a reason for hiding this comment

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

when you do unit testing, usually you avoid randomness for one reason:
if you test failed for a "random" reason if it highly possible that you won't be able to reproduce the issue cause of the randomness of your unit test.

Copy link
Author

Choose a reason for hiding this comment

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

Yes i'm aware that this test is not 100% trusty because of the randomness.
But when I was writing those tests, I was wondering how could we test a function which have infinite inputs. I made some research about it, and find something called "monkey testing".
It was just an attempt to talk about it ^^


const addClassNames = require('../src/SwfObjectProcessor/addClassNames');
const createSymbols = require('../src/SwfObjectProcessor/createSymbols');
const removeSymbols = require('../src/SwfObjectProcessor/removeSymbols');

Choose a reason for hiding this comment

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

for clarity, you should have one test file per js file

Copy link
Author

Choose a reason for hiding this comment

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

Noted, I'll create 3 test files instead of 1 !

@jperdereau
Copy link

cc @bchevalier @tbrebant if you have something to add

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.

2 participants