search

meejah / txtorcon

Twisted-based asynchronous Tor control protocol implementation. Includes unit-tests, examples, state-tracking code and configuration abstraction.
http://fjblvrw2jrxnhtg67qpbzi45r7ofojaoo3orzykesly2j3c2m3htapid.onion/
MIT License
250 stars 72 forks source link

Modify a few tests to use `pytest` #167

Closed felipedau closed 8 years ago

felipedau commented 8 years ago

I now I am late for this, but is it close to what you are looking for the GSoC project?

Analysis

I briefly went through the test/*.py files and modified a few of the simplest tests I found to use fixtures and parametrization. There are a few points which I think that using pytest does improve them.

Readability

In my opinion, convention over configuration is usually confusing for people that do not know the conventions, but once you know them, it gets a lot better than having lots of boilerplate code that you already know what is doing.

Loops

A couple tests were using for loops to make a test run against a set of values. The problem with that is that if one of the values fails, the whole test fails and depending on the error it might not be so easy to understand it. Using parametrized functions/fixtures removes the for loops complexity and a test for each value is run, so that it is simple to find out which value caused the failure, especially due to the test ID:

# previous
test_util.py::TestUnescapeQuotedString::test_invalid_string_unescaping FAILED

# current
test_util.py::test_invalid_string_unescaping[None] FAILED
test_util.py::test_invalid_string_unescaping["""] PASSED
test_util.py::test_invalid_string_unescaping["\"] PASSED
test_util.py::test_invalid_string_unescaping["\\\"] PASSED
test_util.py::test_invalid_string_unescaping["\\""] PASSED

Another case which that might not apply is the test_string_unescape_octals method, where it runs 1000 assertions and parametrizing that would result in 1000 tests, making it harder to read the output.

Assertion messages

Notice that I removed the msg string from the test_valid_string_unescaping function because pytest displays the values that failed the assertion:

>       assert unescaped == correct_unescaped
E       assert 'None' == None

Object reuse

The test_router.py file has the following lines:

controller = object()
router = Router(controller)
router.update("foo",
              "AHhuQ8zFQJdT8l42Axxc6m6kNwI",
              "MAANkj30tnFvmoh7FsjVFr+cmcs",
              "2011-12-16 15:11:34",
              "77.183.225.114",
              "24051", "24052")

Which are duplicated in ~10 tests. Using the updated_router fixture basically allows us to delete those 8 lines from each test case. It might not be the case, but that fixture can also be parametrized, creating even more tests with less code.

Function reuse

The test_valid_lookup_versions function replaced the test_valid_lookup_v2 and test_valid_lookup_v3 methods using a parametrized fixture, saving lines of code and preventing a future reader from reading almost the same code twice.

Lines of code

This commit actually has more additions than deletions (+103/-100) and 3 more lines than previously, but based on the topics mentioned above, I think pytest improved the quality of the tests. Also, I like to think that there is only extra code due to the imports, double line spacing of the main indentation level and creation of constants that could have been avoided by passing the values directly to the parameters, but in my opinion makes the code more readable.

Background

I honestly do not have experience with either Twisted nor txtorcon and I am currently learning pytest for my senior design project, but I am interested in the project. Can the schedule include time to study such things, or it is expected of students to know how everything they are going to do?

Let me know what you think and what you expect for the whole project. For example, what other tools do you expect to use? I am aware of the pytest-twisted package, which might help but does not seem to be popular.

Last thing: what do you have in mind for the TestCase classes? Are they going to be completely replaced?

Thanks!

meejah commented 8 years ago

Yes, this is definitely looking along the right lines; replacing repeated code or loops with @fixtures is great. I have used pytest-twisted before, and it works well -- it's pretty simple.

For the TestCases, yes get rid of them. With py.test I prefer organizing related tests by files, and having just plain function def test_whatever() at the top level.

(And I agree with your comments re: readability vs. lines-of-code -- definitely having a few more "lines" is totally fine if it's more-readable). Thanks for the interest :)

felipedau commented 8 years ago

On Thu, Mar 24, 2016 at 3:00 PM, meejah notifications@github.com wrote:

Yes, this is definitely looking along the right lines; replacing repeated code or loops with @fixtures is great. I have used pytest-twisted before, and it works well -- it's pretty simple.

Cool! Thanks for the fast response!

For the TestCases, yes get rid of them. With py.test I prefer organizing related tests by files, and having just plain function def test_whatever() at the top level.

Alright, sounds good!

(And I agree with your comments re: readability vs. lines-of-code -- definitely having a few more "lines" is totally fine if it's more-readable). Thanks for the interest :)

I am glad you agree, thanks!

I am writing the proposal and I would like to ask you 3 things:

Thanks!

meejah commented 8 years ago

The best thing for a proposal is to be realistic and honest -- if you're not up to speed on twisted/txtorcon (but e.g. do have event-based programming experience), then mention these things and build in time to learn. "usually" the "bonding period" is for coming up to speed on how to interact w/ the community/project etc...

It might indeed not be enough work for a whole summer to 'just' do py.test porting; that would be another thing to evaluate while building a schedule. If you do think it's "light" then build in some "stretch goals" (e.g. making a start/evaluation of the txaio GSoC suggestion, or finishing python3 fixes). Yes, /msg-ing me on OFTC is fine.

meejah commented 8 years ago

(I haven't used the GSoC Web site from a "student" side, but you should have a chance to modify your proposal, so I think you could just put a draft up there -- but do message me to make sure I see it, as there's not much time left for submissions).

felipedau commented 8 years ago

On Thu, Mar 24, 2016 at 3:42 PM, meejah notifications@github.com wrote:

The best thing for a proposal is to be realistic and honest -- if you're not up to speed on twisted/txtorcon (but e.g. do have event-based programming experience), then mention these things and build in time to learn. "usually" the "bonding period" is for coming up to speed on how to interact w/ the community/project etc...

Got it. I think that in 1 month (4/22~5/23) I am able to get up to speed with both Tor's community workflow and twisted/txtorcon.

It might indeed not be enough work for a whole summer to 'just' do py.test porting; that would be another thing to evaluate while building a schedule. If you do think it's "light" then build in some "stretch goals" (e.g. making a start/evaluation of the txaio GSoC suggestion, or finishing python3 fixes).

Well, I guess I will only know the exact complexity of it once I understand everything that is being tested. I will keep reading the docs and I will let you know when I have something to show you.

Yes, /msg-ing me on OFTC is fine.

(I haven't used the GSoC Web site from a "student" side, but you should have a chance to modify your proposal, so I think you could just put a draft up there -- but do message me to make sure I see it, as there's not much time left for submissions).

Yes, I am able to upload and keep updating a draft proposal before sending the final proposal as a PDF. I just wanted to contact you directly because I uploaded a draft on Monday and did not get any feedback yet (probably won't), but apparently because there is no mentor for the idea I proposed.

Well, I hope to contact you soon with something more substantial.

Thanks, @meejah!

meejah commented 8 years ago

I don't believe anyone gets email notificatsion from the GSoC Web site :/ What is your other idea? (Just the title, I can look it up on GSoC)

felipedau commented 8 years ago

On Thu, Mar 24, 2016 at 4:38 PM, meejah notifications@github.com wrote:

I don't believe anyone gets email notificatsion from the GSoC Web site :/ What is your other idea? (Just the title, I can look it up on GSoC)

Hmm, I read somewhere that if someone was interested I would receive a message via one of the contact methods I provided.

It's called "Making Tor more accessible". I can send you the gist if you want.

meejah commented 8 years ago

Did you try reaching out to potential mentors on #tor-dev? There's also a "UX" mailing list now, too...

felipedau commented 8 years ago

On Thu, Mar 24, 2016 at 4:46 PM, meejah notifications@github.com wrote:

Did you try reaching out to potential mentors on #tor-dev? There's also a "UX" mailing list now, too...

The intial idea I had I sent to the tor-assistans list and one of the devs told me he could not find anyone that could help me, but that I should try to reach out on #tor-dev. I talked to a couple devs there, but no one seemed to be interested and I was advised to at least submit the draft the way it was to see if someone might like it, but nothing promising. I'll probably finish it and submit it anyways, but so far the idea is not cool... not sure if I should insist though.

meejah commented 8 years ago

Okay, I see. I guess the bottom line is: a project w/o a mentor simply won't be accepted ... :/

felipedau commented 8 years ago

On Thu, Mar 24, 2016 at 5:00 PM, meejah notifications@github.com wrote:

Okay, I see. I guess the bottom line is: a project w/o a mentor simply won't be accepted ... :/

Yeah, looks like it is. I don't want to spam asking for help but at the same time I wanted to submit something to not feel that I wrote that for nothing :P

Unfortunately I don't even know if someone read the whole thing. Maybe I should've came up with a better title/description...

But thanks for your attention! I will keep reading about txtorcon now :)

meejah commented 8 years ago

I am bringing it up in the UX meeting, which is right now in #tor-project on OFTC

felipedau commented 8 years ago

On Thu, Mar 24, 2016 at 5:11 PM, meejah notifications@github.com wrote:

I am bringing it up in the UX meeting, which is right now in #tor-project on OFTC

Thanks, that's great! I'll be observing it.

meejah commented 8 years ago

This was a demo/proof-of-concept for a GSoC application, so closing it as GSoC application deadline has long passed.