fix compression and tests therefore #108
Conversation
| @@ -54,7 +54,7 @@ object Requester{ | |||
| } | |||
| case class Requester(verb: String, | |||
| sess: BaseSession){ | |||
|
|
|||
| val CMD = verb.toUpperCase // allow submitting as lower case ... | |||
There was a problem hiding this comment.
Why are you adding CMD as a field here?
There was a problem hiding this comment.
"verb" was basically used as an enum, only without insisting it be case dependent. In general it was accessed using by comparing "POST" or something to verb.toUpperCase. One place in the code it was accessed without uppercasing, which I took to be a bug. Maybe I'm old-school, but I don't like code that does the same thing over and over even if the compiler will probably optimize it away, so I changed it to do the upper-casing once and compare to CMD. Maybe there is a style you prefer, but I would fix that bug in any case.
| for((k, v) <- blobHeaders) connection.setRequestProperty(k, v) | ||
|
|
||
| for((k, v) <- sess.headers) connection.setRequestProperty(k, v) | ||
|
|
||
| for((k, v) <- headers) connection.setRequestProperty(k, v) | ||
|
|
||
| for((k, v) <- compress.headers) connection.setRequestProperty(k, v) | ||
| Seq(blobHeaders, sess.headers, headers, compress.headers). | ||
| foreach(h => for((k, v) <- h) connection.setRequestProperty(k, v)) |
There was a problem hiding this comment.
This creates an extra Seq that the old code was avoiding.
I would rollback it, also doesn't help fixing the bug you found.
There was a problem hiding this comment.
Yeah, just a thought
| val bytes = new ByteArrayOutputStream() | ||
| withOs(compress.wrap(bytes)) { os => data.write(os) } | ||
| val byteArray = bytes.toByteArray | ||
| connection.setFixedLengthStreamingMode(byteArray.length) |
There was a problem hiding this comment.
Not sure I see that, but suit yourself.
| /** | ||
| * Do something with an OutputStream and close it | ||
| * @param os OutputStream | ||
| * @param fn | ||
| */ | ||
| private def withOs[T](os: OutputStream)(fn: OutputStream => T) : Unit = | ||
| try fn(os) finally os.close() | ||
|
|
||
|
|
There was a problem hiding this comment.
The Scaladoc here doesn't add much value.
Also I suggest you to rename the method to usingOutputStream to be consistent with Scala 2.13's Using
| /** | |
| * Do something with an OutputStream and close it | |
| * @param os OutputStream | |
| * @param fn | |
| */ | |
| private def withOs[T](os: OutputStream)(fn: OutputStream => T) : Unit = | |
| try fn(os) finally os.close() | |
| private def usingOutputStream[T](os: OutputStream)(fn: OutputStream => T) : Unit = | |
| try fn(os) finally os.close() | |
| test("send"){ | ||
| requests.send("get")("https://httpbin.org/get?hello=world&foo=baz") |
There was a problem hiding this comment.
What happened with formatting here?
There was a problem hiding this comment.
The formatting complaints are more obvious in the editor. Yeah, I can twiddle them.
| import java.net.InetSocketAddress | ||
|
|
||
| import com.sun.net.httpserver.HttpExchange | ||
| import com.sun.net.httpserver.HttpHandler | ||
| import com.sun.net.httpserver.HttpServer | ||
| import java.io._ | ||
| import java.util.zip._ | ||
| import scala.collection.mutable.StringBuilder | ||
| import utest._ | ||
| import ujson._ | ||
| import requests.Compress | ||
| import requests.Compress._ | ||
| import scala.annotation.tailrec |
There was a problem hiding this comment.
Can you organize these imports here?
| // } | ||
| // } | ||
|
|
||
| //Tests fail with 'Request to https://httpbin.org/absolute-redirect/4 failed with status code 404' |
There was a problem hiding this comment.
Did you fix those tests? If so you should remove this comment, right?
| "https://httpbin.org/post", | ||
| compress = requests.Compress.None, | ||
| data = new RequestBlob.ByteSourceRequestBlob("Hello World") | ||
| ) | ||
| assert(res1.text.contains(""""Hello World"""")) | ||
|
|
||
| val res2 = requests.post( | ||
| "https://httpbin.org/post", | ||
| compress = requests.Compress.Gzip, |
| /** | ||
| * Compress with each compression mode and call server. Server expands | ||
| * and passes it back so we can compare | ||
| */ | ||
| test("compressionData") { | ||
| import Compress._ | ||
| val str = "I am deflater mouse" | ||
| Seq(None, Gzip, Deflate).foreach(c => |
There was a problem hiding this comment.
Maybe you could be slightly more specific on what is weird where you don't like the formatting. One of then looks a little over-indented, but I wouldn't know what to change on the others. I've never done this before, and in my imagination you were going to merge in the parts you found agreeable and hit format;-)
| import com.sun.net.httpserver.HttpExchange | ||
| import com.sun.net.httpserver.HttpHandler | ||
| import com.sun.net.httpserver.HttpServer |
There was a problem hiding this comment.
I thought I tested for all versions. I can try again ...
|
Hi @tballard, |
I changed the code to close some output streams so the server can actually get all the data. I added some tests using a local server so it could catch the compressed data, decompress it and send it back to be checked against the original.
Also the code was allowing the verb to be passed in as any case and then checking it by uppercasing and comparing whenever it was used. Except that somebody forgot to upper case it in one place which seemed like a bug. I changed the code to upper case it in one spot.
There were 4 small blocks of code that were trivially reduced to 1 and who could resist?
There were some existing compression tests that were failing once the change was made. The test was to make sure the response data had a specific value. One got the impression that someone just wrote the test after seeing the returned value, but who knows? I changed the tests to match the new returned values.
I uncommented some tests and they didn't blow up, so I left them. Maybe I went rogue ...
I don't think there is anything else of substance.