search

jspm / npm

NPM Location Service
19 stars 34 forks source link

adding tests for lib/auth #97

Closed gustavnikolaj closed 8 years ago

gustavnikolaj commented 8 years ago

As promised, here is my take on testing this project.

When checking out this branch, first run npm install to get the new development dependencies. Then running mocha or ./node_modules/.bin/mocha will run the new tests for lib/auth.js. Alternatively, you can run the tests through instanbul by running npm run coverage and then, the generated coverage report can be found at ./coverage/lcov-report/index.html.

If you have any questions please don't hesitate to reach out here, or on gitter, if you prefer.

I'm using the unexpected assertion framework. It goes hand-in-hand with mocha quite nicely, and gives a really nice API for async assertions, based on promises.

guybedford commented 8 years ago

This is amazing, thank you so so much @gustavnikolaj! No pressure at all on the comments, happy to merge this in manually and do some tweaks here too. I really like the UI Factory pattern... will have to bring this across to jspm at some point :)

gustavnikolaj commented 8 years ago

I tried to follow up on all the comments that you had. I'll wait doing more until I've heard back from you on that.

If you want, I can fix the cases where we expected a rejected promise instead of a fulfilled one with the value undefined. Or we can wait doing that until after it's merged. That's up to you.

I'm glad that you liked the uiFactory. It was the most elegant way I could come up with to mock up the ui interaction, while at the meantime making sure that it's called in the way that you expect. We can easily make a module out of the uiFactory, for reuse in other modules, and even polish it up a bit so it's more general.

guybedford commented 8 years ago

On second thoughts, that case is probably fine. Were there any other cases that you thought seemed like incorrect behaviours? Let me know. If not, then let's merge this.

gustavnikolaj commented 8 years ago

There were two cases, but both being different variants of the same. Shouldn't it return the _auth data instead of undefined then?

guybedford commented 8 years ago

Ah, yes you're completely right.

gustavnikolaj commented 8 years ago

I'll fix that later today and then it'll be ready for merging, I think :-)

gustavnikolaj commented 8 years ago

Done :-)

guybedford commented 8 years ago

Thanks so much for your work on this!

gustavnikolaj commented 8 years ago

You're very welcome. :-) If you have any questions or need a hand with testing in other areas of the project, please don't hesitate to reach out!

guybedford commented 8 years ago

Thanks Gustav, it's very much needed so may well do that!