100% coverage

[hueniverse]

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]

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]

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]

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 outmoded/hapi-contrib#14

[hueniverse]

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

[MylesBorins]

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]

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

[MylesBorins]

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]

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]

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

[AdriVanHoudt]

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

[MylesBorins]

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

[hueniverse]

Leave server.start() in init().

[towns]

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]

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]

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]

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

[chyld]

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]

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]

@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]

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.

[hueniverse]

General comments:

• When monkey patching code in a test, mark that test with the { parallel: false } lab option to make it both safe for future parallel testing as well as visual cue.
• Never return anything other than an actual Error as an error (e.g. no strings, plain objects, etc.).
• Never use fixed port numbers in tests, only 0 or the default.
• Always return before next() unless there is a reason to continue.
• Some of you missed the change in the assignment to make both arguments in init() required.
• When calling server.inject() with a GET request, just pass the path string as the first argument instead of an options object. Makes the code much more readable.
• Use the testing shortcuts boilerplate used in hapi. Makes reading tests easier.

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]

👍

[MylesBorins]

thanks for taking the time!

[zoe-1]

Thanks for the feedback :-)

[gyaresu]

• Monkey patching parallel processing set false 🆗 Thanks @zoe-1 (comment below)
• Only return err 🆗
• Using .equal(8000) in tests. 🆗 (I think)
• Returning the next function: return next() 🆗
• exports.init = function (port, next) { <= That init? from index.js ❓
• Pass the path string: server.inject('/version', function(response) { 🆗
• Testing shortcuts: 🆗

var expect = Code.expect;
var describe = lab.experiment;
var it = lab.test;

[zoe-1]

@gyaresu

lab docs show how to include the parallel option.

lab.test('returns true when 1 + 1 equals 2', { parallel: false }, function (done) {
        Code.expect(1+1).to.equal(2);
        done();
 });

source: https://github.com/hapijs/lab
Got the above from the documentation. Yeah, I did not include it either.

I believe monkey patching would be where we changed the default values
of a plugins methods so it would break. test/index.js

    Version.register = function(server, options, next) {
        next('Plugin Error');
    };

    Version.register.attributes = {name: 'Fake Plugin'};

[tielur]

Use the testing shortcuts boilerplate used in hapi. Makes reading tests easier.

Is that just the Makefile used in the hapi project and/or the npm scripts?

[gyaresu]

@tielur I think it refers to the names you give lab and code methods.
i.e.

var expect = Code.expect; 
var describe = lab.experiment; 
var it = lab.test;

[tielur]

@gyaresu ahh ok that makes sense. Thanks!

[gyaresu]

@tielur I just grabbed your code to have a look and the npm test doesn't print out the tests. Mine did that originally and I'm just looking for the advice somewhere in my PR's. (Will update when I find it but thought you'd be interested as we use different tests)

[zoe-1]

@hueniverse commented:

Some of you missed the change in the assignment to make both arguments in init() required.

I am assuming he wants the parameters in init() to be validated.
Mainly, just test that a valid port was submitted.
Or, is this telling us to not create code that allows for the port to be absent?

How are we to interpret this?

[hueniverse]

The original assignment had the port as optional which meant you had to check if the first argument was a number or a function. This was removed but not everyone got the memo.

[zoe-1]

Ok. got it.
I just made tests for the port again. Will go remove them.

[tielur]

@gyaresu it's the -v option that you have for lab. It does the verbose test output.

[gyaresu]

@tielur Sweet. Thanks.

[ghost]

I have a error that I cannot solve.,
I deleted this part :

internals.init = function () { 
     if (typeof port === 'function') { 

and changed it to :

server.connection({ port: port });

but now on every test I see a error message that port must be a string, a number.
How to solve this ?

My code can be found here : https://github.com/roelof1967/hueniversity

[ghost]

@roelof1967 you have tests that are omitting the port

[ghost]

Thanks, it worked now without any problem. A new PR is submitted already

[AdriVanHoudt]

@hueniverse is it beneficial to add return before the done() calls in every test? I'm not sure if it adds readability. Although I'm personally a fan of it.

[hueniverse]

@AdriVanHoudt Nah.

[AdriVanHoudt]

@hueniverse could you go into why you choose that PR to merge? Also there is now no way for me t run the tests on my windows 😐

[simon-p-r]

You can install make on Windows, I use https://github.com/lukesampson/scoop which is very good for Windows users.

On 19 Apr 2015 10:00 am, AdriVanHoudt notifications@github.com wrote:

@hueniversehttps://github.com/hueniverse could you go into why you choose that PR to merge? Also there is now no way for me t run the tests on my windows [:neutral_face:]

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/79#issuecomment-94252647.

[johnmanko]

So are we creating a Makefile? Also, when you say "Add the standard hapi.js Makefile" please point to documentation on what that "standard hapi.js Makefile" is and how it should be constructed. Too many assumptions on what we know what you're talking about.

[zoe-1]

@johnmank Below is the answer to your makefile question.

As referenced in this assignments post:
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.