Closed lfn3 closed 8 years ago
I really appreciate you digging in and sorting this out - the existing code isn't the easiest to follow. The only really high level suggestion I have is that perhaps testing the request mapping directly (vaguely similar to this, i.e. not issuing the requests) might be more maintainable long term - there are a lot of existing integration tests with data dependencies, which makes things a little awkward. I can make this change if you're strapped for time.
I'll add a couple of questions as line comments.
So I've added a unit test type of deal, let me know if that's kind of in the realm of what you were expecting?
Yeah, totally - I'd be down with removing the integration test - if the unit tests demonstrate the targets you added are being transformed into correct looking requests, I don't think integration tests are worth their weight.
Ok. No problem. I've nixed the integration test.
I'm open to adding some tests to cover some of the other functionality, but I'll put those in another pr if I do.
There's some unused imports in the tests, but it's not worth holding up a merge. I'll push the snapshot shortly.
Deletion was a little odd, since function-name has to be passed in the body of the request, but is actually used in the url. And method always has to be passed as :delete.
These could be wrapped in the utils namespace to make them easier to use, but it's possibly better to alter their use in the core lambda ns.
The base64 string in the test ns was created from the test.js file in the project here: https://github.com/lfn3/node-cljs-zip