Skip to content

Fixed touch(), with time=0 (with tests).#108

Closed
niconoe wants to merge 1 commit intolinsomniac:masterfrom
niconoe:fix_touch
Closed

Fixed touch(), with time=0 (with tests).#108
niconoe wants to merge 1 commit intolinsomniac:masterfrom
niconoe:fix_touch

Conversation

@niconoe
Copy link
Copy Markdown
Contributor

@niconoe niconoe commented Dec 14, 2016

The touch() method was throwing an error when time=0. The error itself wasn't showed properly because of a byte/string issue in a self.debuglog() call.

This PR fix both issues, and provides regression tests.

@timgraham
Copy link
Copy Markdown
Collaborator

I'm not sure if the time.sleep() technique is ideal. Requiring the test suite runtime to increase by multiple seconds when testing timeout doesn't seem like a good path to head down. It might be better to use mocking or to look at other cache libraries for inspiration.

Is it possible to add tests for the debuglog?

@niconoe
Copy link
Copy Markdown
Contributor Author

niconoe commented Dec 15, 2016

About the time.sleep() technique: I agree, but don't really found a simple solution so far, so I thought it was good enough for now. It seems pylibmc use this technique too, pymemcache don't even properly test touch(), and I couldn't find a well-maintained Python-based memcached mock object.

The debuglog() call is implicitly tested on line 87, when trying to touch a nonexistent key (without the fix, TypeError is raised on Python 3). Do you want me to add a comment about this in tests?

Copy link
Copy Markdown
Collaborator

@timgraham timgraham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About verifying debuglog -- I think the tests should capture stderr output and check it rather than the test output looking like:

...MemCached: while expecting 'DELETED', got unexpected response 'NOT_FOUND'
MemCached: while expecting 'DELETED', got unexpected response 'NOT_FOUND'
....MemCached: while expecting 'STORED', got unexpected response 'SERVER_ERROR object too large for cache'
......MemCached: MemCache: inet:127.0.0.1:11211: test.  Marking dead.
.....MemCached: MemCache: inet:memcached:11211: connection closed in readline().  Marking dead.
MemCached: MemCache: inet:memcached:11211 (dead until 1481895267): connection closed in readline().  Marking dead.
.

It's a separate issues though.

Comment thread tests/test_memcache.py
# Delete with explicit time=0 (can re-set immediately)
self.mc.set("my_key", "my_val")
self.mc.delete("my_key", time=0)
self.assertNotEqual(self.mc.set("my_key", "my_val"), 0)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this supposed to be a regression test? As far as I can tell, it's passing before the fix is applied (returning True). Why assertNotEqual rather than assertEqual?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this test because that case (delete with time=0) wasn't covered by the test suite before, and there was a risk my touch() fix would change this behaviour. So I considered it as a safety net, with the added benefit of increasing test coverage.

assertNotEqual(self.mc.set("my_key", "my_val"), 0) to be consistent with the docstrings in memcache.py. It is stated:

  • if delete(time=0), subsequent set() should succeed immediately.
  • a successful set() returns nonzero.

Comment thread tests/test_memcache.py
self.mc.touch("my_key2")
time.sleep(2)
self.assertEqual(self.mc.get("my_key2"), "my_val")
# TODO: test it stays forever
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't seem feasible. Again, I think we should use mocking to verify the values sent to the server without having to wait around to ensure the server handles those values correctly. I'll give it a try if you're stuck.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought using mocking was larger than this issue and could be solved in another PR, but I can give it a try here!

@timgraham
Copy link
Copy Markdown
Collaborator

Updated in #137.

@timgraham timgraham closed this Nov 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants