Setting up Scalatest-Play for 1.6.0-SNAPSHOT

[Will Sargent]

Hi all,

Unless there are any objections, I'm going to change the master branch of scalatest-play to 1.6.0-SNAPSHOT in a bit:

https://github.com/playframework/scalatestplus-play/pull/52

And after that, I'm going to make some breaking changes that will mean that functional tests in Scalatest will need to incorporate an application explicitly, This means that people who are using compile time dependency injection or other forms of DI don't have to look under the hood for the guice dependency, which has been a source of confusion.

This is done either on the class, by mixing in GuiceFakeApplicationFactory, or using GuiceOneAppPerTest, or by overriding def fakeApplication(): Application to provide your own "fake" application.  

https://github.com/playframework/scalatestplus-play/pull/53/

This corresponds to the work that Greg is doing to break out Guice from Play:

https://github.com/playframework/playframework/pull/6178

This does means that any patch releases will have to be made on the master branch and explicitly backported, but there is already a 1.5.x branch so I don't think there's any process change there.

https://github.com/playframework/scalatestplus-play/tree/1.5.x

Will.

[James Roper]

Hi Will,

Could you give a summary of how it will break users code?  What changes will need to be made?  Will it be a change to every test?  Is there a deprecated migration path?

Regards,

James

--

James Roper
Software Engineer

Lightbend – Build reactive apps!
Twitter: @jroper

[Will Sargent]

So where you would have 

class ExampleSpec extends OneAppPerTest {
   ...
}

if you use the code in 1.5.x it will work with no problems, because the GuiceApplicationBuilder is defined under the hood.  In 1.6.x, that will cause an exception because fakeApplication() is an abstract method and 1.6.x does not define a default definition.

If you define the test as 

class ExampleSpec extends GuiceOneAppPerTest {

}

that will provide the Guice application by default.

The migration path should be a straight search and replace, but migration with deprecation warnings is a little tricky because there's nothing "wrong" with OneAppPerTest except for having a default builder.  One possibility might be to deprecate OneAppPerTest et al, then make GuiceOneAppPerTest extend AbstractOneAppPerTest (a functional clone of OneAppPerTest) so that there isn't a direct dependence on deprecated trait.

Will.

[Rich Dougherty]

So that users don't need to implement the fakeApplication method would it be possible to introduce a new base class, kind of like how we introduced EssentialAction when we decided to generalise Action?

BaseOneAppPerTest <-- move most of existing OneAppPerTest code here

+--GuiceOneAppPerTest <-- move Guice code here

|  +--OneAppPerTest <-- users can stay using this class; deprecate eventually

+--SpringOneAppPerTest <-- example; not provided by Play

+--DaggerOneAppPerTest <-- example; not provided by Play

--
You received this message because you are subscribed to the Google Groups "Play framework dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to play-framework-...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

--

Rich Dougherty

Engineer, Lightbend, Inc

@richdougherty

[Will Sargent]

Yep, looks like we came up with roughly same plan... I'll add BaseOneAppPerTest in.

[Greg Methvin]

This seems more of job for a separate mixin trait (if that is possible):

OneAppPerTest with GuiceSupport

And mixing in that trait is equivalent to overriding the `newAppForTest` method, which would be present in all the main traits.

This gets interesting when you think about how compile-time DI apps should be constructed. The compile-time DI support trait should have another place where you create the components, like the example here: https://github.com/playframework/play-macwire-di/blob/master/test/WithApplicationComponents.scala#L24

As far as the naming for the new traits, maybe AppPerTest or PlayAppPerTest?

Greg Methvin

Senior Software Engineer

Lightbend, Inc.

[Will Sargent]

> As far as the naming for the new traits, maybe AppPerTest or PlayAppPerTest?

I prefer BaseAppPerTest or AbstractAppPerTest, as it makes it clear the trait needs a mixin before it will work correctly.  Not everything uses "newAppForTest" though.

The compile time DI apps would have to use their own mixins, as done in the example project:

https://github.com/playframework/play-scala-compile-di-with-tests/tree/master/test 

Will.

[Greg Methvin]

I was suggesting changing all the traits to use the same mechanism for getting a new app, like we did in the example project, so it's easy to mix in traits to override that functionality.

[Will Sargent]

That mechanism is being used under the hood:

trait FakeApplicationFactory {

  def fakeApplication(): Application

}

with the implementation:

trait GuiceFakeApplicationFactory extends FakeApplicationFactory {

  def fakeApplication(): Application = new GuiceApplicationBuilder().build()

}

And you can use it that way:

 

class AbstractAppPerTest with GuiceFakeApplicationFactory

The migration path would mean specifically adding that trait to rather than changing OneAppPerTest to GuiceOneAppPerTest though...

Will.

[Greg Methvin]

Well, it's easy to create a single trait by mixing the two together:

trait GuiceAppPerTest extends AppPerTest with GuiceApplicationFactory

So you can make the convenience traits if you want for migration, but IMO multiple traits seems more flexible. Also you could define the abstract AppPerTest trait to be:

trait AppPerTest { self: ApplicationFactory => }

So, if you are using compile-time DI or some other scheme, it's obvious that all you have to do is create and mix in your application factory.

[Will Sargent]

I've committed changes based on Greg review:

https://github.com/playframework/scalatestplus-play/pull/53/

The main change is adding the FakeApplicationFactory as an explicitly typed self reference so we can call fakeApplication() from the Base* traits.

Will.