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

Mocha tests and global leak #8

Merged
merged 6 commits into from
Mar 19, 2013
Merged

Conversation

nisaacson
Copy link
Contributor

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
Copy link
Contributor

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 600a657 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.

…oved redundant generation of salt2 in async test, added node_modules/ to .gitignore since this is a shared module, not one that is deployed directly
@nisaacson nisaacson closed this Mar 19, 2013
@nisaacson nisaacson reopened this Mar 19, 2013
@nisaacson
Copy link
Contributor Author

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
Copy link
Owner

👍 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
Copy link
Contributor Author

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
Copy link
Owner

ah, yep. Typo :P

shaneMangudi added a commit that referenced this pull request Mar 19, 2013
Mocha tests and global leak
@shaneMangudi shaneMangudi merged commit 1bb6bf1 into shaneMangudi:master Mar 19, 2013
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.

None yet

3 participants