outmoded / university

Community learning experiment
Other
371 stars 193 forks source link

100% coverage #79

Closed hueniverse closed 9 years ago

hueniverse commented 9 years ago

Things are getting a bit more interesting...

It's time to add tests, verify coverage, confirm style, and automate all of this with CI. We will be using the lab module to perform all these tasks and automate it with travis.

  1. Add a .travis.yml file, testing our project on node 0.10, 0.12, and io.js (latest).
  2. Add a test folder with two files, version.js and index.js, each testing the corresponding file under /lib.
  3. Modify the package.json file to include the tests, as well as the dev dependency to lab.
  4. Add the standard hapi.js Makefile to make it easy to generate other types of reports (e.g. HTML coverage report). Note: From Assignments5 on this project only uses npm. Between assignments 4 and 5, style rules changed and the Makefile was removed from hapijs projects.
  5. When using lab, enable coverage, require 100% coverage, enable linting with default rules, and use the code assertion library.
  6. Write a basic test to verify our version endpoint in version.js.
  7. Change the init() method to accept a port and a callback. Use the callback to return when the function completes or errors. The init() callback should return any error state as well as a reference to the newly created server. This will allow us to later stop the server when we test it.
  8. Export init() and move the invocation to a new start.js file (which will call the init() function with the 8000 port and a callback the outputs the information to the console when started). Change the package.json file to use the start.js file as the starting place. This file will not be covered by tests.
  9. Write a basic test to verify the init() function in index.js.
  10. Bring coverage to 100% by adding more tests as needed.

Everything up to (10) should be pretty straight forward. If you are not sure on how to use lab and code, look at other hapi.js modules like hoek, qs, items, and boom (e.g. simple modules) to copy their test scripts and setup.

Getting 100% coverage can be tricky sometimes so if you are not sure, get as much coverage as you can, and comment on the lines in your pull request where you are having a hard time reaching and someone will give you a clue.

Remember to properly stop() your servers when calling the init() method in each test.

For now, avoid using any of the before() and after() lab features.

As always, ask for help and help others!

Due: 4/4

AdriVanHoudt commented 9 years ago
  1. What is the thought behind defining the response in internals? And requiring the package in there to? In index we specifically required Version at the top to then use it?
  2. Wasn't there a general agreement(if you want I'll search for the issue in question) to use npm for all the different tests as in define them in package.json instead of a Makefile?
hueniverse commented 9 years ago
  1. Fixed.
  2. The makefile gives you easy access to stuff like make cov-html while the package.json one is for npm test.
AdriVanHoudt commented 9 years ago
  1. Ok but you still declare the response in internals, why?
  2. you can easily run npm run cov-html plus it works also on Windows

EDIT related issue https://github.com/hapijs/contrib/issues/14

hueniverse commented 9 years ago

The response is in internals so you don't create the object on every request. It's a small optimization.

MylesBorins commented 9 years ago

I'm a bit stuck with the logic of testing when no arguments are given to init. Specifically I am curious the best practice on what init should return. If init does not return the server then there is no way to shut it down in testing without using something for spying.

For right now I have opted to return the server, it is gross but hopefully it will work for now

hueniverse commented 9 years ago

@TheAlphaNerd did you notice the steps above to modify the init() method?

MylesBorins commented 9 years ago

Indeed. You specifically state If no callback is provided, assert on errors.

I think I managed to figure out how to get that to work as expected with coverage. The only thing I'm stuck on now is figuring out how to test specific error states that we will be executing the callback from.

For example how do I write a test that guarantees the plugin will load with an error.

MylesBorins commented 9 years ago

It is worth mentioning that the solution I found was returning the server object when no callback is provided. What confused me specifically was how to properly stop the server if not callback was provided

hueniverse commented 9 years ago

I have changed the assignment (step 7 and 8) to avoid the problem you hit. Sorry about that.

AdriVanHoudt commented 9 years ago

@hueniverse I can see the optimization but does it weigh up against less readability? Also last thoughts on the makefile?

MylesBorins commented 9 years ago

@hueniverse one more quick clarification. Do we want to call server.start() in start.js or lib/index?

hueniverse commented 9 years ago

Leave server.start() in init().

towns commented 9 years ago

What about the travis integration? Do we need to set that up? I have added the travis.yml file, but do I need to configure my project in Travis to verify that I have automated this properly?

MylesBorins commented 9 years ago

the .travis file is all you need!

On Mar 25, 2015, at 10:37 AM, John Townsend notifications@github.com wrote:

What about the travis integration? Do we need to set that up? I have added the travis.yml file, but do I need to configure my project in Travis to verify that I have automated this properly?

— Reply to this email directly or view it on GitHub.

zoe-1 commented 9 years ago

Question and Solution Regarding Item 7: The init() callback should return any error state as well as a reference to the newly created server. This will allow us to later stop the server when we test it.

Modified original post and replaced it with the below observations. Much thanks to @TheAlphaNerd for his input.

Originally, I was not clear about why or how to face the above item7 issue. But, after doing more research and dialoguing with @TheAlphaNerd I have learned: That using a callback to handle errors allows you to write "throw safe" code passing errors up the call back chain.

When writing error handling code either you pass the error up the call back chain or just make the application shutdown. Passing errors up the callback chain makes your errors "throw safe" and does not asplode the person's application when your module does not work. In contrast to passing errors up the callback chain, you can use: Hoek.assert(); This will just shut the whole application down if there is an error and return an error message.

Item7 just wants us to make init() "throw safe". With throw safe code, if a module does not load properly the error will not shut the whole application down. Hopefully, this will help someone when addressing item7 in the assignment list.

If you want to see a good example of this check out @TheAlphaNerd's assignment3 PR ./lib/index.js. Or, @chyld's ./lib/index.js. They illustrate "throw safe" error handling with callbacks.

If I made a mistake above, please correct my observations.
Any help to master this body of knowledge is much appreciated :-)

rutaihwa commented 9 years ago

Community, I am bit more than stuck starting from point 6, I could use some help to get going.

chyld commented 9 years ago

To @zoe-1's point, the code in index.js and version.js should not be throwing any errors, with Hoek, but instead should be sending any errors back to the original caller - which would be either start.js or in the test code.

mikejerome commented 9 years ago

Is there a way to have the linter display the filenames where linting errors are found? Mine only shows line numbers but no filenames.

hussainanjar commented 9 years ago

@mikejerome linter does show both filename and line numbers, see below example output of linter

Linting results:
    lib/index.js:
        Line 15: semi - Missing semicolon.
    lib/version.js:
        Line 15: no-trailing-spaces - Trailing spaces not allowed.
mikejerome commented 9 years ago

Hmmm I'm using solarized terminal. The filenames must be the same color as my background or something. I verified that they are there by copy-pasting but they are invisible in my terminal.

Edit: I've confirmed it looks like an issue with my terminal theme the filenames are there. Thanks.

zoe-1 commented 9 years ago

Tutorial for Assignment3 now in the project wiki! https://github.com/hueniverse/hueniversity/wiki/3-Assignment look it over, make recommendations, and contribute :-)

chyld commented 9 years ago

Nice work @zoe-1

rutaihwa commented 9 years ago

Thanks @zoe-1

hueniverse commented 9 years ago

@zoe-1 feel free to put it all directly in this project wiki.

zoe-1 commented 9 years ago

@hueniverse I put Assignment3 wiki tutorial into this project. Plus, I made some more edits. I manually copied and pasted it into the project. Is there a way to automate that? kind of like a PR for a wiki? Thank you for starting hueniversity :-) Cheers!

hueniverse commented 9 years ago

So far we got half the previous number of pull request. If you have submitted a PR before but didn't this time, what's the reason? Is it too hard? Lost interest?

Any feedback would be useful to improve the experiment.

MylesBorins commented 9 years ago

I think it is worth considering the fact that while there are less PR's there was quite a bit more activity

MrBri commented 9 years ago

I have a love-hate relationship with writing tests. I know its important and it feels good to see the green but the desire to sit down and write them isn't as strong as say creating something new for a user.

mikejerome commented 9 years ago

I'm a total noob who has never done testing but I know I need to do it and following along has made it much less intimidating. We are migrating some legacy stuff to a single page app and are using hapi for the api. This is an amazing project and I am learning so much from this experiment. I feel like this is so much more powerful than blog posts or videos or even books. I feel extremely lucky to have gotten in near the beginning since I randomly found this on the gitter chat while asking a question. I check back constantly for updates on everything. Thanks @zoe-1 I think your wiki is super awesome.

AdriVanHoudt commented 9 years ago

I think the size + difficulty might have to do something with it. The assignment looks bigger, although the actual execution size is not much more than before, just more smaller things. The main thing would be difficulty, the basic tests are fine but getting to 100% cc can be hard (my, and others, solution was to switch out the register function of the version plugin) and to me that doesn't seem like a clean test and also not super straight forward. On the other hand I think the project itself has indeed more activity on it as @TheAlphaNerd says. @MrBri I'm not gonna discuss the importance of tests with you but doing some basic ones like in here might help you create some more love for them ;)

rutaihwa commented 9 years ago

Just as @AdriVanHoudt when i read the specification, everything seemed so out of my league. As with many programmer who like to hack not to test. But in fact, The entire thing makes alot of sense. Maybe I should adopt this philosopy of starting with tests and then write the actual code. I think @hueniverse statement: "Everything up to (10) should be pretty straight forward." may have made many some people who were not very sure with the stuff like my self a bit nervous, I guess.

AdriVanHoudt commented 9 years ago

also side note, the assignment didn't state where start.js needed to go @rutaihwa good to hear I'm not alone, it comes down to writing style maybe? When reading the assignment it felt like a lot and hard but when broken down it was not to hard as Everything up to (10) should be pretty straight forward suggests, although stuff like that does not help insecure devs who might have problems along the way(which is totally fine, we're here to help) and then read that what they are struggling with should be straightforward

gyaresu commented 9 years ago

I'm a sysadmin who has done a whole bunch of 'learn JS' work and codewars/project euler problems. Lack of knowledge around aspects such as the way lab works or how to structure the app (cb chain) meant that I am still reading about the pieces rather than using them to build the app. I'm still following along as this is really valuable but it just might be at a more confused pace.

I don't want to be disingenuous by just copypasta'ing someone elses work.

Thanks to @zoe-1 for the wiki, that moved the challenge from "no clue" to "what to research".

hussainanjar commented 9 years ago

@gyaresu this is community based learning, you will still learn better with copy pasting then not doing any thing, its not a competition so no one will be caught for cheating, better advice don't copy paste but try to write code following others pull request if its too difficult for you do it yourself right now.

Though learning and making an honest attempt to do it yourself is the best way to go.

AdriVanHoudt commented 9 years ago

Completely true, I tried as mich as I can on my own but for testing the plugin registration failiour catch I also looked at how other did it

On Tuesday, 31 March 2015, Hussain Fakhruddin notifications@github.com wrote:

@gyaresu https://github.com/gyaresu this is community based learning, you will still learn better with copy pasting then not doing any thing, its not a competition so no one will be caught for cheating, better advice don't copy paste but try to write code following others pull request if its too difficult for you do it yourself right now.

Though learning and making an honest attempt to do it yourself is the best way to go.

— Reply to this email directly or view it on GitHub https://github.com/hueniverse/hueniversity/issues/79#issuecomment-88152537 .

zoe-1 commented 9 years ago

After reading Assignment3 requirements, I think I understood about 60% of it. (Possibly, many just quit here) Since I did not understand the whole assignment, I began to look through PRs for sample code I could learn from. I studied @TheAlphaNerd's and @chyld's code the most and read a bunch of documentation.

After that, I wrote my own program based on what the above two did, posted my project, and got feedback from others. Interestingly, the people I modeled my code after critiqued my project :-) I learned a ton from this assignment:-) Thank you to everyone who contributed. Hopefully, more people will join and continue with the assignments.
There is still time to submit a PR!

@AdriVanHoudt and @hussainanjar are right, we should study other PRs to complete assignments. If you rely on or use others' work, just give them credit for their influence on your work. Plus, if you are afraid of copying and not learning while studying someone else's code, just re-type everything and do not copy and paste. While re-typing the code make sure you understand everything you type. Look up every command in the docs etc...

gyaresu commented 9 years ago

Thanks @zoe-1 I'm attacking it with fresh eyes.

zoe-1 commented 9 years ago

@gyaresu go for it!

hueniverse commented 9 years ago

I've extended the deadline to 4/4 in hopes more people to give it a try. If you are not clear, just look at other pull requests as people above suggested. The way this entire experiment works is by moving the learning from the assignment instructions to the community interaction.

If you do not interact, you will not learn and you will fall behind. We are all teachers here!

zoe-1 commented 9 years ago

Just made a document called wiki plan. Hope it sparks some good conversation and planning :-) Edited: Moved Wiki Discussion document from wiki to Gist: now at: https://gist.github.com/zoe-1/a1052051d49fb5bd88c2

Please take a look.

AdriVanHoudt commented 9 years ago

@zoe-1 nice work :+1: , I think the wiki needs it's own issue to discuss things, right now anyone can edit and there is no real feedback system on it :/ maybe even worth it to make a separate repo or something for it

zoe-1 commented 9 years ago

@AdriVanHoudt just made an issue for the wiki.

zoe-1 commented 9 years ago

Wrote a short explanation of how assignment3 used singleton objects made with node's required modules. This concept is essential to understand how to avoid exporting values just to pass tests. See: https://gist.github.com/zoe-1/d5465682428edc583ccc

Plus, wrote an explanation of why we want to use a Makefile. https://gist.github.com/zoe-1/784100440f0cf2299010 Hope these help.

ghost commented 9 years ago

I don't think the purpose of a makefile, at least in the context of this assignment, was to create an alias to npm commands; We were asked to use the standard hapi makefile, which is pretty specific, and we probably shouldn't go changing it to match our personal preferences. Maybe @hueniverse can elaborate on why hapi chooses this method, but beyond that, we should just embrace it. @zoe-1 the singletons article is great, and your doing awesome work ^_^

zoe-1 commented 9 years ago

@EvanMarkHopkins thank you for your kind words :-)

In respect to the Makefile, like you said, hopefully @hueniverse will explain why he wants it in the project. Personally, I view it as a convenience for abbreviating commands frequently used. Based on the following hapijs issues https://github.com/hapijs/contrib/issues/14 and https://github.com/hapijs/lab/issues/284, it seems that @hueniverse is ok with removing the Makefile and putting everything into npm. So, I suspect it is used as a convenience and is just a preference of @hueniverse to use. But, that is only my guess.

ghost commented 9 years ago

Okay, I just found it really bizarre. Nothing about it really suggested a preference for interfacing with Make directly, it just seemed like a valid way to organize things. I don't mind either way, but I can see this causing unnecessary confusion if we keep it just because.

hueniverse commented 9 years ago

Re: makefile - I wanted people to see the two options of setting command shotcuts with npm and make. hapi uses both and the trend is to move away from the makefile and only use npm shortcuts. But I find it useful to have both so that you can see how it's done but also able to find your way around other projects that use either option.

AdriVanHoudt commented 9 years ago

@hueniverse thanks for the reaction. I see the purpose of learning to use both but it's ok to let the makefile point to npm in this case right?

hueniverse commented 9 years ago

General comments:

Also, if others are leaving comments on your code, you should either reply or address their suggestions. This experiment works by learning sideways, not just top to bottom. I have seen a lot of great comments from peers, so please respect their time and use it to learn. If I see you ignoring useful comments from others, I am not going to bother adding my own comments.

gyaresu commented 9 years ago

:+1: