New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Override TwitterModule in FeatureTest #233

Closed
thomas-rc-hutchinson opened this Issue Aug 25, 2015 · 8 comments

Comments

4 participants
@thomas-rc-hutchinson

thomas-rc-hutchinson commented Aug 25, 2015

My initial approach to create the server in my FeatureTest was

override val server = new EmbeddedHttpServer(new MyServer)

I soon realised that creating a new MyServer for multiple tests (running in the same JVM) led to issues with static (or more precicesly object) values. For example HttpServer uses the trait com.twitter.server.Stats. Stats looks on the classpath for implementations of StatsReceiver, it picks up the codehale one (that my project uses) found here https://github.com/rlazoti/finagle-metrics. This library uses a singleton object MetricsStatsReceiver, on test 1 metrics are registered, on test 2 they are registered again and it fails because they are already registered.

java.lang.IllegalArgumentException: A metric named jvm.heap.committed already exists
    at com.codahale.metrics.MetricRegistry.register(MetricRegistry.java:91)

Then I read the following

Server and Router

The Finatra convention is to create a Scala object with a name ending in "Main" as so

object MyServerMain extends MyServer

class MyServer extends HttpServer {...}

This allows your server to be instantiated multiple times in tests without worrying about static state persisting across test runs in the same JVM. MyServerMain is then the static object which contains the runnable main method.

https://github.com/twitter/finatra/blob/master/http/README.md

In short it says use a singleton for the tests to avoid issues with static state. This is great but by doing this I can't override TwitterModules on a per test basis and the server is only intaliated once. For example I inject classes that return error responses and throw exceptions.

Is there anyway to use the singleton server and override Twitter modules?

@thomas-rc-hutchinson

This comment has been minimized.

thomas-rc-hutchinson commented Aug 25, 2015

Just realised that the singleton can not be used.

java.lang.IllegalArgumentException: requirement failed: app must be a new instance rather than a singleton (e.g. "new FooServer" instead of "FooServerMain" where FooServerMain is defined as "object FooServerMain extends FooServer"
    at scala.Predef$.require(Predef.scala:219)

My code

override val server = new EmbeddedHttpServer(MyServerMain)

object MyServerMain extends MyServer
class MyServer(module: TwitterModule = MyModuleImpl) extends HttpServer {
@edma2

This comment has been minimized.

Contributor

edma2 commented Aug 25, 2015

We indeed want to avoid having singletons in tests, but in some cases the underlying library (e.g. Finagle) makes certain assumptions about what can be singletons.

Can you try excluding the codahale stats library in your tests, and only include in your deploy jar?

@schrepfler

This comment has been minimized.

schrepfler commented Aug 25, 2015

I think that would be pretty complicated, we have code in the main launcher which kicks off exposure of coda hale metrics JmxReporter.forRegistry(MetricsStatsReceiver.metrics).build() and this would probably break once we remove the dependency from the classpath.

@scosenza

This comment has been minimized.

Contributor

scosenza commented Aug 26, 2015

So it appears that codahale's metric library is more strict regarding duplicate metric registrations than the finagle-stats metrics library that we use internally at Twitter. I'll take a look at codahale's library and see if I can find a suitable workaround.

@scosenza

This comment has been minimized.

Contributor

scosenza commented Aug 26, 2015

Can you try clearing the metrics after each test? e.g.

def afterEach() {
  super.afterEach()
  MetricStatsReceiver.metrics.removeMatching(MetricFilter.ALL)
}
@schrepfler

This comment has been minimized.

schrepfler commented Aug 27, 2015

Not much luck with that, still investigating.

@scosenza

This comment has been minimized.

Contributor

scosenza commented Sep 6, 2015

Hi @schrepfler. I just tried using the afterEach method posted above, and my multiple tests pass as long as the tests are run sequentially. If using SBT, you can disable parallel tests like so:

parallelExecution in ThisBuild := false

Please let us know if this also works for you.

Thanks,
Steve

@scosenza

This comment has been minimized.

Contributor

scosenza commented Sep 17, 2015

I also updated one of our examples to use Codahale metrics. With parallel test execution disabled, the multiple feature tests appear to be working.
See https://github.com/twitter/finatra/blob/v2.0.0/examples/hello-world-heroku/src/test/scala/com/twitter/hello/HelloWorldFeatureTest.scala#L14

I'm closing the issue for now, but please reopen if you have any further issues.

@scosenza scosenza closed this Sep 17, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment