JSilver99/gamesaver — comment on commit 22f8a8948ecc9ebb7f822758c098c0240c037dc0
File:  line 0
By dmose:

This looks great; I'm just about ready to land it, except that the test has now caught a bug (w00t! TDD for the win :-), and it's generally a really good invariant to never land anything that leaves the tests in a broken state, because that then causes anyone else who updates to the head to become blocked.

It's possible that this bug is related to the fact that I recently updated to Node 0.6.0.

Here's the test output:

Running all GameSaver server tests...

> GameSaver@0.0.1 test /Users/dmose/s/games/gamesaver
> node test/*-test.js

·✗ ✗ ✗

Testing the GameSaver Saving/Loading... A POST to /api/login
✗ Login should return the email upon verification
» expected 'testEmail@example.com',
got null (===) // save-test.js:21

Testing the GameSaver Saving/Loading... A POST to /api/set
✗ should respond with 200
» expected 200,
got 400 (==) // api-easy.js:233

Testing the GameSaver Saving/Loading... A GET to /api/get?gameTitle=TestGame
✗ Should receive the data we just sent for TestGame
SyntaxError: Unexpected token B
at Object.parse (native)
at Object. (/Users/dmose/s/games/gamesaver/test/save-test.js:32:33)
at Request. (/Users/dmose/s/games/gamesaver/node_modules/api-easy/lib/api-easy.js:225:16)
at runTest (/Users/dmose/s/games/gamesaver/node_modules/api-easy/node_modules/vows/lib/vows.js:91:26)
at EventEmitter. (/Users/dmose/s/games/gamesaver/node_modules/api-easy/node_modules/vows/lib/vows.js:71:9)
at EventEmitter.emit (events.js:88:20)
at /Users/dmose/s/games/gamesaver/node_modules/api-easy/node_modules/vows/lib/vows/context.js:32:52
at Request._callback (/Users/dmose/s/games/gamesaver/node_modules/api-easy/node_modules/vows/lib/vows/context.js:46:29)
at Request.callback (/Users/dmose/s/games/gamesaver/node_modules/api-easy/node_modules/request/main.js:99:22)
at Request. (/Users/dmose/s/games/gamesaver/node_modules/api-easy/node_modules/request/main.js:361:18)
✗ Errored » 1 honored ∙ 2 broken ∙ 1 errored (0.322s)

I added a logging statement to the server code, and the stderr there shows this before it aborts:

listening on 127.0.0.1:54321
data = assertion=&audience=localhost%3A54321
verifying assertion!
{"status":"failure","reason":"need assertion and audience"}
failed to verify assertion: need assertion and audience

Turns out what's happening is that on line 28 of main.js, localHostname is defaulted to undefined, and then on line 160, it's used to construct a fallback "audience" string when the request doesn't include a "HOST" header.

This suggests to me that we actually want two tests here, one that explicitly sends a HOST header and one that explicitly doesn't. The current code should pass the first test as is, and we need to figure out a better default behavior in order to fix the second one. My first thought is to default to the node equivalent to gethostname(3). If that turns out to be hard, we could ask the DNS.