Removing specs from finagle/finagle-native test. #294
Conversation
Problem In order to upgrade to 2.11 (and as part of a larger twitter initiative) specs has to be removed. This is one small step in doing so. Solution Remove specs, replace with the standard FunSuite / assert way of testing. Result. Hopefully nothing will change. Except for removing specs.
| "openssl-root.conf" | ||
| ) | ||
|
|
||
| def init() { |
There was a problem hiding this comment.
instead of calling an init method, the style we typically use is to either make a Context class that we construct and import members from, or make a wrapper method that can do the construction for us. this means that we don't have to worry about sharing objects.
For example:
class Ctx {
val input = ...
}
val ctx = new Ctx()
import ctx._or
def exec(fn: Input => Unit) {
...
}
exec { input =>
}There was a problem hiding this comment.
Alright, I'll switch it over to that style.
|
Is that test failure something local to the server it ran on? I don't get that locally. |
|
Hmm, yeah, it's a very weird failure. It hasn't happened historically, though. My guess is it's just flaky. LGTM |
| ) | ||
|
|
||
| class Context { | ||
| // before we run any tests, construct the chain |
There was a problem hiding this comment.
There's no need for a Context class, you should just run the code inside Context before executing the test.
(I think this is the cause of the test failure on TravisCI)
There was a problem hiding this comment.
Haha, I literally told him the opposite. I like having a Context class because it makes it clear that you're not sharing any mutable state. Why do you think it would cause a test failure?
There was a problem hiding this comment.
The CertChainOutput that you create at line 56 will fail because you never create this context (never execute the constructor to be precise).
In general, I agree with you, but here you don't share anything with the instance of Context, you're just counting on the side effect of creating an instance.
There was a problem hiding this comment.
Just to make sure I'm reading this right you're saying to just run the code in Context, rather then newing up the class? Shouldn't the block in the class be ran upon instantiation?
There was a problem hiding this comment.
You read me correctly.
The block in the class should be ran before initializing CertChainOutput.
|
Updated, does it look better now? |
|
It does look better! |
|
Now on GitHub. |
Going along with the corresponding issue #290 and my other PR on the subject #292