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

Adding Travis for testing, fixing minor PIP violations. #40

Merged
merged 8 commits into from
Oct 8, 2015

Conversation

joshmarshall
Copy link
Owner

In order to support other people actually contributing sanely, I've added the first step of Travis integration so it runs the (minimal) existing tests in both 2.6 and 2.7. As I went through I made minor whitespace corrections and any test changes necessary to run with nosetests in both unittest and unittest2 test cases.

Feedback desired!

@efokschaner
Copy link
Contributor

👍
The changes look fine.

I had a few observations about the tests for the future:

  1. You could make the test server port dynamic (in the ephemeral range) by binding to 0 and then retrieving it with socket.getsockname() which nicely avoids any potential for conflict.
  2. I notice that a lot of the validation is done through the history system. It would be a more "black box" test to verify the client using the raw data that the server receives.
  3. To reduce the likelihood of non-compliance in the server masking non-compliance in the client, you could avoid running an implementation of jsonrpc on the server and instead just use some pretty static response logic. This would then also allow you to easily have some less "happy-path" tests where the server intentionally responds abnormally. The drawback with this route is you'd probably then want separate tests for your server implementation.

If you want, I can apply the above ideas in the tests that I'll be adding for the _Method changes unless you'd prefer consistency with the other tests. Let me know.

module without any parameters to run the tests.

Currently, this is not easily tested with a framework like
Currently, this is not easily tested with a framework like
Copy link
Contributor

Choose a reason for hiding this comment

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

This paragraph is a little contradictory now that you are using nose.

@joshmarshall
Copy link
Owner Author

@efokschaner

You bring up several very good points -- to be honest, this project has remained mostly unchanged for years, and I wrote it pretty early in my Python learning. :) It needs a fairly thorough overhaul to be brought up to a decent standard. Your notes highlight some of that. I've made a couple of the changes (fixed the message, and moved to dynamic ports) you recommended, if you'd like to make some tweaks yourself I certainly would appreciate it!

That being said, your change is currently blocked by this PR, and this PR would help move the project into a healthier process (if not fixing some of the issues themselves). Whenever we feel like this is "good enough" to move to next step, we should probably do that and then tackle next cleanup items.

Thoughts?

@efokschaner
Copy link
Contributor

@joshmarshall I fully agree this is good enough. My notes were just ideas for the future. I added a comment in the source regarding the port stuff.

I'm still 👍 on this PR with or without any further changes.

joshmarshall added a commit that referenced this pull request Oct 8, 2015
Adding Travis for testing, fixing minor PIP violations.
@joshmarshall joshmarshall merged commit 10bea44 into master Oct 8, 2015
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