search

vigetlabs / university

Community learning experiment
1 stars 4 forks source link

JF: Assignment #1 #7

Closed jeremyfrank closed 8 years ago

jeremyfrank commented 8 years ago

I got the server setup and running pretty easily, but I had trouble with properly setting up testing in a way that had a shared server setup. I had to resort to looking at other PRs and borrowed some ideas from them for setting up testing.

nhunzaker commented 8 years ago

Other than adding hapi as a dependency, lookin good! :+1: :tada:

mackermedia commented 8 years ago

Looks good :+1:. Guessing you don't have 100% test coverage since that error line in the server.start() isn't covered. I had the same issue and just deleted the logging of the error. I thiiiink I saw it should be somewhat easily doable with something I saw in the server.inject() API docs.

jeremyfrank commented 8 years ago

@mackermedia think this would cover it? 119db2c

mackermedia commented 8 years ago

Oh, no I meant that you aren't actually testing that line that throws an error: https://github.com/vigetlabs/university/pull/7/files#diff-6d162108fa01dae8b09c5d9116006eb4R4

nhunzaker commented 8 years ago

@jeremyfrank @mackermedia This line feels hard to test productively. Currently there isn't sufficient code that would generate an error to begin with.

My example I was able to get code coverage to bump to 100% by using assert.ifError:

https://github.com/vigetlabs/university/blob/master/submissions/nhunzaker/lib/index.js#L5

I do not think that this is fantastic whatsoever. Specifically it does not test that the server truly fails if an error is thrown. However in future assignments, we'll add the ability to extend the server with additional behaviors, which will provide plenty of opportunity to break things.

I'm cool punting until then. Any other method to reach that code feels contrived.

:+1:

nhunzaker commented 8 years ago

Forgot one thing. In addition to using assert.ifError, I'm verifying that any errors are properly forwarded to the callback I provide for creating servers using this test:

https://github.com/vigetlabs/university/blob/master/submissions/nhunzaker/test/server.test.js#L31-L40

But still, let's let future assignments bring the opportunity instead of chasing after it.

mackermedia commented 8 years ago

Agreed. Thx for ur thoughts.

On Tue, Jan 12, 2016 at 1:16 PM, Nathan Hunzaker notifications@github.com wrote:

Forgot one think. In addition to using assert.ifError, I'm verifying that any errors are properly forward to the callback I provide for creating servers using this test:

https://github.com/vigetlabs/university/blob/master/submissions/nhunzaker/test/server.test.js#L31-L40

But still, let's let future assignments bring the opportunity instead of chasing after it.

— Reply to this email directly or view it on GitHub https://github.com/vigetlabs/university/pull/7#issuecomment-171021036.