Skip to content

Unit test - #40

Open
Solinca wants to merge 18 commits into
Wizcorp:masterfrom
Solinca:unit-test
Open

Unit test#40
Solinca wants to merge 18 commits into
Wizcorp:masterfrom
Solinca:unit-test

Conversation

@Solinca

@Solinca Solinca commented Aug 5, 2018

Copy link
Copy Markdown

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

@jperdereau jperdereau left a comment

Copy link
Copy Markdown

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

Comment thread package.json Outdated
},
"scripts": {
"test": "NODE_ENV=test mocha './**/*.test.js'",
"istanbul": "istanbul cover _mocha $(find ./ -name \"*.test.js\" -not -path \"./node_modules/*\")"

Copy link
Copy Markdown

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

@Solinca Solinca Aug 7, 2018

Copy link
Copy Markdown
Author

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

Comment thread unit-tests/SwfObjectProcessor.test.js Outdated
it('add two or more class name', function () {
const min = 2;
const max = 5;
const random = Math.floor(Math.random() * (max - min + 1)) + min;

Copy link
Copy Markdown

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
Copy Markdown
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 ^^

Comment thread unit-tests/SwfObjectProcessor.test.js Outdated

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

Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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