monix / shade

Memcached client for Scala
MIT License
106 stars 19 forks source link

Complete failed status translations with tryComplete #39

Closed lloydmeta closed 8 years ago

lloydmeta commented 8 years ago

In reference to @alexandru's comment in #38 , try to complete the future when faced with untranslatable statuses instead of throwing.

Also:

coveralls commented 8 years ago

Coverage Status

Coverage decreased (-0.9%) to 83.636% when pulling f5be74e6942dfd43dfc3bd84ab6d82871bcbc8f1 on lloydmeta:refactor/spy-memcached-impl into 8e7d871f09bbdbcf4994cdf7af7f5152b353878c on alexandru:master.

lloydmeta commented 8 years ago

Hah, coverage decreased because I added lines to handle cases that should never happen (missed translation of status...)

alexandru commented 8 years ago

Does it work? :-)

coveralls commented 8 years ago

Coverage Status

Coverage increased (+1.5%) to 86.071% when pulling acfa56317ef29f064538091889a2d0b074bfa956 on lloydmeta:refactor/spy-memcached-impl into 8e7d871f09bbdbcf4994cdf7af7f5152b353878c on alexandru:master.

lloydmeta commented 8 years ago

Ah, was still polishing the PR :) DRY'ed it up a bit more to make failure handling a bit less repetitive on the SpyMemcachedIntegration level and now we have +1.5% coverage.

Does it work? :-)

So far the tests are passing, which is a good sign. Since it's a bit hard to force the underlying SpyMemcached to throw unsupported statuses, I manually disabled handling of valid cases in realAsyncGet, ran the tests again, and saw that we got the following:

[info]   shade.UnhandledStatusException: For key hello - shade.memcached.internals.CASNotFoundStatus$
[info]   at shade.memcached.MemcachedImpl.shade$memcached$MemcachedImpl$$throwExceptionOn(MemcachedImpl.scala:291)

which looks like the proper failure path in the MemcachedImpl layer that we want. Looking at the previous code, I don't think we would have ever been able to get to MemcachedImpl.scala:291, since as you mentioned, the future would never get completed due to error throwing !

alexandru commented 8 years ago

Dude, so overall looks good.

alexandru commented 8 years ago

👍 so if you think it's good, you can merge.