search

bbc / alephant-broker

Brokers requests for alephant components
MIT License
4 stars 3 forks source link

Batch requests #3

Closed Integralist closed 10 years ago

Integralist commented 10 years ago

nobody-got-time-for-that ^ our users

Integralist commented 10 years ago

@kenoir can you give a interim review of this PR please.

I need to utilise the status, not_found and error request objects as I realised I'm not using those even though I've created them.

I might need to do a bit of refactoring (I'll check shortly).

Then I'll crack on with updating/writing tests (I think it might be quicker to actually rewrite the tests as they're all breaking now)

Integralist commented 10 years ago

@kenoir heya, I've got a couple more tests to write (for the new Request objects) but in the mean time could you have a look at https://github.com/BBC-News/alephant-broker/blob/5020bb5f528b98d3583d1760eb2c053f53453e39/spec/batch_response_spec.rb as I personally think it smells but I'm not sure what to do about it.

The reason I think it smells is because there is a lot of stubbing and set-up going on to test a single assertion. Doesn't seem right. Like I should be able to do a single stub/mock on an object rather than stubbing individual instance attributes

cc @stevenjack

kenoir commented 10 years ago

@integralist you're stubbing the dependencies, of which there are many. You should see the stublicious horror I created today. This smells sweet by comparison, don't worry.

kenoir commented 10 years ago

@integralist that said there might be some rspec methods to help you out, maybe 'instance_double' and 'as_null_object'? Maybe also AWS.stub! Would work for you? (Disclaimer: I couldn't get any of them to work for me)

Integralist commented 10 years ago

@kenoir cool thanks I'll take a look at those after I've finished writing up the other tests (just so I have something working)

Integralist commented 10 years ago

@kenoir this is ready to be reviewed