search

kpdyer / fteproxy

programmable proxy for censorship circumvention
https://fteproxy.org/
Apache License 2.0
149 stars 21 forks source link

Refactor cli #180

Closed irdan closed 6 years ago

irdan commented 8 years ago

This PR more cleanly separates arg parsing and execution of code. It also extracts repeated coded into functions, and addresses some flake8 style issues.

There are a lot of changes here so I'm happy to address any issues or nitpicks.

kpdyer commented 8 years ago

Awesome. Thanks! I'll be able to review this early next week.

kpdyer commented 8 years ago

I've had a quick look at the changes. Superficially they all look reasonable and are definitely in the right direction.

However, with these code changes, an innocuous typo (e.g., renaming a CLI param.) can break fteproxy for lots of users. The most popular use case is with Tor [1], but it will probably be quite challenging to mimic the Tor Browser environment.

As an alternative, we may want to just test fteproxy in a few a cases: (1) with only client|server specified, (2) with IP addresses, and (3) with keys.

It'd be great if you could address this. If not, I'll have time to work on it this weekend.

[1] https://gitweb.torproject.org/builders/tor-browser-bundle.git/tree/Bundle-Data/PTConfigs/linux/torrc-defaults-appendix#n2 [2] https://travis-ci.org/kpdyer/fteproxy

irdan commented 8 years ago

I would be happy to write test cases covering these changes and more. Is there an established method of measuring test coverage?

I have lots of experience with testing in Javascript but almost none in python. Is there any documentation on writing tests for fteproxy? Even a simple outline would go a long ways in getting me up to speed.

kpdyer commented 8 years ago

The idea is that we'll be able to write unit tests that are executed when you run python setup.py test.

I'll present an example this weekend. However, it will roughly look something like following:

https://github.com/kpdyer/fteproxy/blob/master/fteproxy/tests/test_relay.py

but instead of invoking the client/server via the start() method, they'd be invoked via the command line.

leggewie commented 6 years ago

when can we expect this to land?

irdan commented 6 years ago

@kpdyer I'm happy to pick this up again. Can you elaborate on your preferred approach to testing?

kpdyer commented 6 years ago

fteproxy usage for Tor these days is so low that we won't really break anyone if make a non-backwards compatible change. AFAICT, everything here looks good. Thanks!