shaneMangudi / bcrypt-nodejs

Native implementation of bcrypt for NodeJS
Other
574 stars 69 forks source link

Mocha tests and global leak #8

Closed nisaacson closed 11 years ago

nisaacson commented 11 years ago

I added a mocha-based test suite so you can run npm test. Both the async and sync tests are implemented.

Also in bCrypt.js there were a few var statement errors that I cleaned up

NicolasPelletier commented 11 years ago

As you can see in #4, I have played with this package a bit and like it much so I wanted to see the goodies you added ( more tests is always a good thing ).

Good catch on the leak.

I cloned your fork ( https://github.com/nisaacson/bcrypt-nodejs ) to run the test. I could not because there are missing dependencies: 'async' and 'eyespect'. You should probably add them to the list of devDependencies.

Also, the async-test.js file in 600a6577670923ded9f6bc9151197950a79c36b3 the value of 'salt2' is assigned twice ( lines 31 and 75 ). One of them is redundant. I think you can remove the function at line 71 altogether. I did locally and the tests are still working.

Finally, you could remove the origin test files in the root folder since they are replaced by your test files... Or maybe not for those who want a simple example they can Copy&Paste in they own code. I guess I leave that up to @shaneGirish.

nisaacson commented 11 years ago

I made the changes you suggested. I left the original test files in the project root folder. You can decide whether or not to keep them.

The naming of the individual async tests could be cleaned up. The actual functionality being tested is correct but the output displayed to the user of bacons and veggies is not that indicative of what logic actually being tested

shaneMangudi commented 11 years ago

:+1: Awesome :)

Just one thing though, the 'npm_modules' folder seems to be included, shouldn't they be taken out before I merge this in ?

nisaacson commented 11 years ago

I am assuming you mean node_modules. I think I added it by mistake by doing a git add -A. Go ahead and take it out.

I added node_modules to the .gitignore in the latest commit to avoid this in the future http://www.futurealoof.com/posts/nodemodules-in-git.html

shaneMangudi commented 11 years ago

ah, yep. Typo :P