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

Full test suite #3

Merged
merged 2 commits into from
May 9, 2016
Merged

Full test suite #3

merged 2 commits into from
May 9, 2016

Conversation

dawsbot
Copy link
Contributor

@dawsbot dawsbot commented May 9, 2016

I hear you want tests. So I built tests for your tests so you can test while you test.

dawsbot and others added 2 commits May 8, 2016 22:37
@simonlovesyou
Copy link
Owner

Thanks for the PR! :)

I will merge this PR into a test branch, and then write tests and when the tests are done merge it to master.

This PR would've been best separated into two PR's though, one with the syntax highlighting and one with the addition of a test suite, so that I could've merged just the README additions right away, but this is fine 👍

@simonlovesyou
Copy link
Owner

I've added a simple test to your PR here, and I will now merge it to master :) Thanks for your help!

@simonlovesyou simonlovesyou merged commit 294fc10 into simonlovesyou:master May 9, 2016
@dawsbot
Copy link
Contributor Author

dawsbot commented May 9, 2016

👍

@dawsbot
Copy link
Contributor Author

dawsbot commented May 10, 2016

@simonlovesyou you removed the test to ensure that the file was removed? From what I can see, you changed the naming convention of the file and removed my check that the file was indeed removed. Is that correct?

https://github.com/simonlovesyou/file-wipe/blob/test/test/index.js

@dawsbot
Copy link
Contributor Author

dawsbot commented May 10, 2016

In addition, in the original PR, I left a test for the promisified version commented out. That was failing, perhaps I was testing your promisified version incorrectly, but it looked like promises were not setup properly with the library.

@simonlovesyou
Copy link
Owner

I couldn't find any test in your PR :( Might have been from an old .gitignore entry that I used when tests weren't done yet. Feel free to submit a new PR with your test if you'd like!

Also, forgot about the naming convention that you've put in place. I'll fix that soon.

I'm going to have to look into that, seems to be working for me. The tests that are in place right now doesn't test the callback though.

Sorry about the mess 😅

@dawsbot
Copy link
Contributor Author

dawsbot commented May 10, 2016

Oh, the mess is my fault with the testing, I didn't PR you correctly, never even pushed by tests.

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

2 participants