Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some of the test names were a little confusing. #3

Open
apokalyptik opened this issue Dec 29, 2014 · 4 comments
Open

Some of the test names were a little confusing. #3

apokalyptik opened this issue Dec 29, 2014 · 4 comments

Comments

@apokalyptik
Copy link

You have protocol validation tests under the lockd client heading in tests which made me thing that you were testing features of the client.

eg "serializes operations on a single client" made me think you were trying to do some magic to make multiple instances share the same connection. And "forbids a client to get a lock another client already holds" made me think you were keeping an internal list of locks and having multiple clients check to avoid sending a command to the server.

Not wrong, but confusing as I searched for that in the code to verify what was going on :)

@nylen
Copy link
Owner

nylen commented Jan 2, 2015

Ah, right. I'll work out how to organize the tests better.

The one about serializing is a feature of the client library: since commands may return multiple lines, it performs server requests in series. See relevant code and test lines, and I've added some documentation. How can I word this better / make it more clear what's being tested?

@apokalyptik
Copy link
Author

How about something like "Concurrent requests are properly serialized inside a client" ?

nylen added a commit that referenced this issue Jan 2, 2015
@nylen
Copy link
Owner

nylen commented Jan 2, 2015

Done. I'll leave this open until there are more tests and I've organized them better.

@nylen
Copy link
Owner

nylen commented Jan 19, 2015

The tests are better organized now, but that same test (serializes concurrent requests from a single client) still sticks out as being misplaced.

It's an important test, though, and this is definitely the easiest place to put it since there will be a real lockd server running at that point. So, I'm inclined to leave it alone for now and leave this issue open.

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

No branches or pull requests

2 participants