python-tls / tls

A pure Python implementation of the Transport Layer Security protocol version 1.2, using existing libraries for crypto math.
Other
163 stars 44 forks source link

Hypothesis tests for the PRF #96

Closed markrwilliams closed 7 years ago

markrwilliams commented 8 years ago

This PR introduces two optional Hypothesis tests for the PRF.

They won't run unless you've got Hypothesis installed, and they're also not configured to run under tox. That's because Hypothesis stores test history in a special directory so it can retry known failures. I think it makes sense for these to be opt-in only, requiring that you run py.test from the project's root directory.

I hope they're pretty straightforward to follow. Please ask questions!

markrwilliams commented 8 years ago

OK, this needs a better way to skip hypothesis tests. I was toying with the idea of writing a marker and maybe adding some extras to setuptools. More commits forthcoming.

Ayrx commented 8 years ago

I don't think the tests should be opt-in only. Realistically unless they are part of the full test suite they will never be run often enough.

markrwilliams commented 8 years ago

@Ayrx Thanks for the feedback!

I don't think the tests should be opt-in only. Realistically unless they are part of the full test suite they will never be run often enough.

That's a fair point. My concern is that by design they take a while to run, and I think it's important to maintain a test suite that covers the core functionality 100% but also runs as fast as possible. Concretely, the entire existing test suite runs in 0.6 seconds on my laptop, while the hypothesis tests alone run in 5 seconds. I'm not quite at Gary Bernhardt's level, where I'm having the tests run every time I save a file, but I'm considering it :)

I'm thinking right now that I'll rig up a way to skip the tests by default, so that they appear as "s"s in the run output, and then to add a tox environment that runs them. If the CI job takes a little longer that's perfectly OK, and it means that developers don't have to sit through the hypothesis tests to get good feedback, but those tests will run automatically, somewhere. I'd really like it if there were a way to persist the .hypothesis directory between CI runs, but as far as I know that's an open problem, and not one I intend to solve as part of this PR!

Does it seem OK if they're run only on the CI by default?

Ayrx commented 8 years ago

@markrwilliams If test time is the main concern, it can be a good idea to organize the hypothesis tests under a single subdirectory and have the main set of tox targets ignore it by passing the --ignore flag to pytest. Then have several (1 for each supported Python version) "hypothesis-only" tox targets that runs the tests in that specific subdirectory.

ashfall commented 8 years ago

I'm thinking right now that I'll rig up a way to skip the tests by default, so that they appear as "s"s in the run output, and then to add a tox environment that runs them.

Sounds good.

Does it seem OK if they're run only on the CI by default?

Yes, as long as they are run automatically somewhere after every change (a.k.a. CI ) . :)

Thanks @markrwilliams !

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling f74e180be571029a7c502f205f1842c2c74d9a18 on markrwilliams:hypothesis-prf into 166d6591a9e9246f774e8ef6ab0a295f891e8b30 on pyca:master.

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling d1184ac011722f325b928b70a1ffaf4549d976af on markrwilliams:hypothesis-prf into 166d6591a9e9246f774e8ef6ab0a295f891e8b30 on pyca:master.

markrwilliams commented 7 years ago

@ashfall This is ready for review!

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 037978e68c452f1a3024ddc16cbd2507cc6b115a on markrwilliams:hypothesis-prf into 166d6591a9e9246f774e8ef6ab0a295f891e8b30 on pyca:master.

ashfall commented 7 years ago

Looks great to me. I also made a note to self to add py35 builds separately, which I hope to get to this weekend, if not sooner. Thanks for working on this! I'm happy to merge if you are also. :)

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling f09a652615049e21ac683aa795443a15da9e595f on markrwilliams:hypothesis-prf into 166d6591a9e9246f774e8ef6ab0a295f891e8b30 on pyca:master.

markrwilliams commented 7 years ago

@ashfall Thanks so much for the great review and general patience with this PR! Now that the build's green I'll merge.