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 unit tests - utilities and default options schema check #34

Merged
merged 15 commits into from
Jun 30, 2016

Conversation

xrysanthos
Copy link
Contributor

I have created an initial set of Mocha tests covering two sections:

  • default options validity based on a schema
  • unit test for amqp-util module

More unit tests will follow for the rest of the modules.

@codecov-io
Copy link

codecov-io commented Jun 6, 2016

Current coverage is 100%

No coverage report found for develop at 3b24984.

Powered by Codecov. Last updated by 3b24984...8d2ddc2

@nfantone nfantone self-assigned this Jun 6, 2016
@nfantone nfantone added this to the 1.0.0 milestone Jun 6, 2016
@nfantone
Copy link
Collaborator

nfantone commented Jun 6, 2016

@chefArchitect Want me to review and merge? Or shall I wait until you finish pushing changes?

@nfantone nfantone mentioned this pull request Jun 6, 2016
4 tasks
@xrysanthos
Copy link
Contributor Author

I suggest we do this in small iterations. Check this one first and in the following days I'll be pushing more updates.

@nfantone
Copy link
Collaborator

nfantone commented Jun 8, 2016

@chefArchitect Reviewing this now.

@@ -14,5 +14,7 @@ setInterval(function() {
client.act('role:create', {
max: 100,
min: 25
}, console.log);
}, function(err, msg) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would you mind not mixing stylistic changes with functional changes on the PR? 🐰

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry @nfantone - my bad. Forgot to rollback this one.

chefArchitect added 2 commits June 8, 2016 18:24
* Fixed lint related issues
* Reverted examples/client.js to original
@xrysanthos
Copy link
Contributor Author

@nfantone Just pushed all latest unit tests + linting issue fixes. If you want me to write tests that cover Hooks related functionality, let me know.

@nfantone
Copy link
Collaborator

nfantone commented Jun 8, 2016

Good stuff.

I'll take a look, ASAIC.

@xrysanthos
Copy link
Contributor Author

Hi @nfantone. Are you planning to merge this PR anytime soon or you want me to close it? Didn't have any feedback from you for a while now.

@nfantone
Copy link
Collaborator

I'm sorry for the long silence. I've been unusually busy lately.

I'm all up for this. I'll try to review it before tomorrow.
On Jun 29, 2016 9:51 AM, "Chris Spiliotopoulos" notifications@github.com
wrote:

Hi @nfantone https://github.com/nfantone. Are you planning to merge
this PR anytime soon or you want me to close it? Didn't have any feedback
from you for a while now.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#34 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAdDpcgFHulL4Jg5QYaHXCinTY_FFa9oks5qQmpngaJpZM4IvGnY
.

@xrysanthos
Copy link
Contributor Author

Cool! Thanks.

@nfantone
Copy link
Collaborator

@chefArchitect Ok, so first of all: amazing work. Thank you very much for your efforts and contribution.

Everything LGTM. I can't ask you to do anything more than what you already did.

Just two lil' comments before merging this.

  1. I know the IDE should probably bug you about it (I'm looking at you, Atom) - but, following the general eslint-config-seneca rules on gulpfile.js is not very graceful to the eye. Using Gulp instead of gulp looks just plain silly. As long as gulp validate passes, everything else is fine to me. So, could you revert that, please?
  2. Would you please rebase your PR branch? There have been two new commits to develop since.

@xrysanthos
Copy link
Contributor Author

xrysanthos commented Jun 30, 2016

@nfantone Ok, we're cool now. Synced wit the 'develop' branch and reverted the gulp config, excluding it also from linting. Sorry for the inconvenience.

@nfantone nfantone merged commit d594ada into senecajs:develop Jun 30, 2016
@nfantone
Copy link
Collaborator

nfantone commented Jun 30, 2016

There, done.

@chefArchitect Thanks again. I'll update the docs to add a contributors list with your name on it.

EDIT: I owe you a 🍺

@xrysanthos
Copy link
Contributor Author

@nfantone If you come by Athens anytime soon, let me know :)

@xrysanthos xrysanthos deleted the mocha-unit-tests branch June 30, 2016 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants