Add call to sitespeed #3
Conversation
This directory will hold the results of the sitespeed call.
rmparr
force-pushed
the
add-call-to-sitespeed
branch
from
f937cdb to
5d7f310
Compare
tests/callSitespeed.test.js
Outdated
| @@ -0,0 +1,7 @@ | |||
| const callSitespeed = require('../src/callSitespeed'); | |||
|
|
|||
| test('the response is good', async () => { | |||
There was a problem hiding this comment.
In the Ruby world, we have libraries for mocking out expensive external API calls. I believe this should be done here as well, otherwise this test suite will quickly become unmanageable.
There was a problem hiding this comment.
It's not a very pleasant test for sure. There is not much to test aside from the actual API call, though. Mocking it would basically involve setting errors = [], and then testing if errors.length = 0.
What I think would be a reasonable solution would be to move the test out of the actual function and into an expected long-running integration test. The integration test would still take 20-30 seconds.
There was a problem hiding this comment.
Ryan, this is looking good, and just a couple changes are needed before I'd merge it in - thanks!
There's a comment from Martin about testing, and mocking the call out to sitespeed.
While it's useful for now as a starting point , jest supports mocking quite well, and I'm happy to chat about in more detail.
👍
src/index.js
Outdated
| const msg = 'no Pub/Sub message received'; | ||
| console.error(`error: ${msg}`); | ||
| return h.response(`Bad Request: ${msg}`).code(400) | ||
| if (!url) { |
There was a problem hiding this comment.
We're not doing any checking about whether this is a valid url at the mo - would you please have a go using Joi?
I think there's an example in the code showing how to use Joi, the validation library. Can you please look over this and make this reject a request if we don't have a valid url passed in the POST body/payload?
This tutorial should help.
There was a problem hiding this comment.
I opted for a small function with a regex. Aiming to limit the dependencies as much as possible, this was 4 lines vs 1.7mb.
This function is writing to the tmp directory, which will have to be rsynced to a remote directory.
rmparr
force-pushed
the
add-call-to-sitespeed
branch
from
5d7f310 to
41ccbc3
Compare
The handler now sends an async call to the callSitespeed function, returning either a list of errors or a success message. The handler now also validates a correctly formed URL is passed in. The entire server is exported so it can be tested.
The tests confirm bad requests return failures, and that a good request should return success.
rmparr
force-pushed
the
add-call-to-sitespeed
branch
from
41ccbc3 to
6ff9532
Compare
|
Hi Ryan, this is looking good. I'm happy to merge this in, as further feedback about coding styles and so on after likely best done when we have some explicit guidelines to refer to, to stop them being too subjective :) Nice work 👍 |
This is a PR for issue #1 .
I have written a function to call sitespeed.io and return either the results or the errors.
I have also updated the index.js route handler to confirm a URL was provided, and to asynchronously return either the errors or a success message from the call to sitespeed.
There is also a small (and long running) test on the callSitespeed function, but this can probably be improved upon.
Currently, this is writing the responses of the sitespeed call to the tmp directory. This can be changed, or we can write something to ship the data to another location.