Write proxy-go tests to cover existing implementation

Currently we only have proxy-go tests to test extracting the client's remote IP from the SDP offer.

We should increase test coverage.

Trac:
Parent Ticket: #29259 (moved)

added actualpoints::2 anti-censorship-roadmap-september component::circumvention/snowflake owner::cohosh parent::29259 points::2 priority::medium resolution::fixed severity::normal sponsor::28 status::closed tests type::defect labels

Here's a start: https://github.com/cohosh/snowflake/compare/proxy_tests

I refactored some of the proxy-go code to use an http.RoundTripper to communicate with the broker. I think it looks a bit cleaner this way in addition to making tests easier.

Trac:
Owner: N/A to cohosh
Status: new to assigned

Looks good so far. The http.RoundTripper makes sense. RoundTrip is slightly different than Do in that it doesn't follow redirects, but I think we want RoundTrip behavior anyway.

Trac:
Keywords: snowflake deleted, anti-censorship-roadmap-september added
Points: N/A to 2

I rebased these tests after merging #28942 (moved), but I'm going to put this on hold until we merge #29207 (moved) since that makes significant changes to how the proxies communicate with the broker and will therefore require further changes to the tests.

Here's some work for improving proxy-go test coverage: https://github.com/cohosh/snowflake/compare/bug29207...ticket30867

I modified the proxy-go code a bit in this commit to use an http.RoundTripper as discussed above.

The test coverage is now at 37.2% (up from 2.2%). I don't think the rest of the code makes sense to write tests for in its current form since that would require some larger refactors.

Trac:
Status: assigned to needs_review

Trac:
Actualpoints: N/A to 2

Looks good, thanks. I would suggest moving SDP, sampleSDP, sampleOffer, and sampleAnswer inside TestBrokerInteractions where they are used.

Incidentally, the new tests reveal that some functions are probably logging at too low a level -- I normally wouldn't expect the kind of code dealt with in unit tests to log.

2019/11/09 13:53:45 invalid character 'e' in literal true (expecting 'r')
.2019/11/09 13:53:45 Cannot deserialize SessionDescription without sdp field.
.2019/11/09 13:53:45 Cannot deserialize SessionDescription without type field.
.2019/11/09 13:53:45 Unknown SDP type

...2019/11/09 13:53:45 io.Copy inside CopyLoop generated an error: io: read/write on closed pipe
2019/11/09 13:53:45 io.Copy inside CopyLoop generated an error: io: read/write on closed pipe

Trac:
Status: needs_review to merge_ready

Merged in 742070a7fb: https://gitweb.torproject.org/pluggable-transports/snowflake.git/commit/?id=742070a7fbeef8e8dc0c68060a12266fe60e0ba0

Replying to dcf:

Incidentally, the new tests reveal that some functions are probably logging at too low a level -- I normally wouldn't expect the kind of code dealt with in unit tests to log. {{{ 2019/11/09 13:53:45 invalid character 'e' in literal true (expecting 'r') .2019/11/09 13:53:45 Cannot deserialize SessionDescription without sdp field. .2019/11/09 13:53:45 Cannot deserialize SessionDescription without type field. .2019/11/09 13:53:45 Unknown SDP type

...2019/11/09 13:53:45 io.Copy inside CopyLoop generated an error: io: read/write on closed pipe 2019/11/09 13:53:45 io.Copy inside CopyLoop generated an error: io: read/write on closed pipe }}} I agree, I think we should not be logging errors generated by CopyLoop since one loop will always close the pipe and trigger an error in the other loop. I'll make a new ticket for this since there are some errors being logged in the broker after #29207 (moved) that are too much at the moment.

Trac:
Status: merge_ready to closed
Resolution: N/A to fixed

closed

changed time estimate to 16h

added 16h of time spent

mentioned in issue #31794 (moved)

mentioned in issue #29259 (moved)

moved to tpo/anti-censorship/pluggable-transports/snowflake#30867 (closed)