Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Run-mode auto-detection enhancement #1466

Closed
wants to merge 6 commits into from

5 participants

@japgolly
  • Allow customisation.
  • Added extra to list of known test runners.

https://groups.google.com/forum/#!topic/liftweb/e3VDUwL7_80

japgolly added some commits
@japgolly japgolly Made run-mode auto-detection customisable.
This allows users to compliment or override the default logic used to
determine the run-mode (ie. dev/test/staging/etc) in absence of a
"run.mode" system variable.

An example use case would be a user using test framework that Lift
doesn't know about, and it undesirably running in development mode.
7106c72
@japgolly japgolly Use test run-mode when running tests from ScalaTest or IDEA. 67384f2
@japgolly japgolly Signed contributors.md 47dc23b
.../liftweb/util/PropertyWithModificationCondition.scala
@@ -0,0 +1,12 @@
+package net.liftweb.util
+
+/**
+ * A property that can be modified under limited conditions.
+ */
+abstract class PropertyWithModificationCondition[T](initialValue: T) {
+ @volatile private var value = initialValue
+ def get = value
+ def set(newValue: T): Boolean = if (allowModification) {value = newValue; true} else {onModificationProhibited; false}
@farmdawgnation Collaborator

Let's split this up into multiple lines for clarity, and loose the semicolons when we do.

Cool, will do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
.../liftweb/util/PropertyWithModificationCondition.scala
@@ -0,0 +1,12 @@
+package net.liftweb.util
+
+/**
+ * A property that can be modified under limited conditions.
+ */
+abstract class PropertyWithModificationCondition[T](initialValue: T) {
+ @volatile private var value = initialValue
+ def get = value
+ def set(newValue: T): Boolean = if (allowModification) {value = newValue; true} else {onModificationProhibited; false}
+ def allowModification: Boolean
+ def onModificationProhibited() {}
@farmdawgnation Collaborator

Parens and brackets shouldn't be required here. (A return type might be though.)

Do you mean def onModificationProhibited: Unit ?
I could do that but then it would make it abstract; I was thinking we might not always want to override it.

The parens are just convention because it's a side-effecting method. The method can still be invoked without parens but the other way around (ie. no parens here and parens at call-site) is prohibited.

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

I like it. Want to give some other committers a chance to look at it too, but I can't find anything else in the code I want to nitpick on. I'm wondering if we shouldn't add some basic tests for the new classes introduced, though.

Great work. Thanks for the contribution!

.../liftweb/util/PropertyWithModificationCondition.scala
@@ -0,0 +1,28 @@
+package net.liftweb.util
+
+/**
+ * A property that can be modified under limited conditions.
+ */
+abstract class PropertyWithModificationCondition[T](initialValue: T) {
@Shadowfiend Owner

I'm mildly concerned that this is overly abstracted. Are there other places we're going to be wanting to use this? Vs just specifying the given functionality in RunModeProperty?

@lkuczera Collaborator

Interesting idea.

@japgolly
japgolly added a note

I'm not sure. I'm still relatively new to Lift but my thought-process was this: I'm seeing lots of settings (esp. LiftRules) that live in singletons. For runtime you just set them once in Boot and it's happy days but when it comes to testing, there've been a few cases I've run into where the singleton settings will initialise before a test can change a needed setting. Easy to fix once you're aware of it, but the problem I had was I'm not always aware of it and sometimes it can take a while to pinpoint the problem. I was thinking if we could let the user know when attempting to set a property that it's futile, it would save effort.

By RunModeProperty do you mean just adding something like warnIfRunModeInitialised() and getting rid of PropertyWithModificationCondition?

@japgolly
japgolly added a note

Sorry, my brain had a CRC error, I get you now.
I thought other parts of LiftRules could use it eventually but I can't think of an specific examples right now. On the other hand, if we remove it now it will be easy to resurrect later when needed.

@Shadowfiend Owner

Yeah. I'd say let's drop it for now, wrap it into RunModeProperty, and keep it in mind in case we run into a similar use case elsewhere in LiftRules.

That said, I'm willing to wait a little bit to see if anyone can come up with an existing place where we can use it now before we rip it out. If we do choose to leave it in, I think we should see if we can hook up some tests to it like @farmdawgnation suggested.

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

I like it. Want to give some other committers a chance to look at it too, but I can't find anything else in the code I want to nitpick on. I'm wondering if we shouldn't add some basic tests for the new classes introduced, though.
Great work. Thanks for the contribution!

No, thank you! I'm thoroughly in love with Lift since discovering it and it's my pleasure to be able to contribute :)

Re tests, yes, I agree. I thought about it but I'm at a bit of a loss at how I would test runModeInitialised being false. It's set when the lazy val runMode is initialised and that's on a singleton... I'm happy to write one but do you have any advice for how?

@farmdawgnation
Collaborator

Maybe testing the run mode stuff specifically won't be something we should try testing, but perhaps adding basic specs for PropertyWithModificationCondition would be possible?

@japgolly

Cool, I'll take another look in a day or two and see what I can do. I should be able to come up with something.

@japgolly

Alrighty, I removed PropertyWithModificationCondition and added tests for modification.

It'd be nice to test run-mode detection in full but I don't think that will be possible without larger, more intrusive changes to Props and/or Props.runMode. I also tried using LiftRulesMocker as Diego suggested but Props is outside of LiftRule's scope, and unaffected. It would take some serious wizardry to be able to restore vals in an object to their default values anyway ... crazy reflection and hacking would be required.

@fmpwizard
Owner

:+1: Looks great! I'll wait a day or two and then I'll merge this one and the mailer pull request (which will give some merge conflicts so I'll just address them locally and push them to master.

Thanks for the contributions!

@Shadowfiend
Owner

:+1:

@fmpwizard
Owner

rebased to master, thanks!

@fmpwizard fmpwizard closed this
@japgolly

Awesome, thanks guys!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jun 27, 2013
  1. @japgolly

    Made run-mode auto-detection customisable.

    japgolly authored
    This allows users to compliment or override the default logic used to
    determine the run-mode (ie. dev/test/staging/etc) in absence of a
    "run.mode" system variable.
    
    An example use case would be a user using test framework that Lift
    doesn't know about, and it undesirably running in development mode.
  2. @japgolly
  3. @japgolly

    Signed contributors.md

    japgolly authored
Commits on Jun 28, 2013
  1. @japgolly
Commits on Jul 3, 2013
  1. @japgolly
  2. @japgolly
This page is out of date. Refresh to see the latest.
View
6 contributors.md
@@ -168,3 +168,9 @@ Will Palmeri
### Email: ###
wpalmeri at gmail dot com
+### Name: ###
+David Barri
+
+### Email: ###
+japgolly @@ gmail .. com
+
View
90 core/util/src/main/scala/net/liftweb/util/Props.scala
@@ -27,7 +27,7 @@ import common._
* Configuration management utilities.
*
* If you want to provide a configuration file for a subset of your application
- * or for a specifig environment, Lift expects configuration files to be named
+ * or for a specific environment, Lift expects configuration files to be named
* in a manner relating to the context in which they are being used. The standard
* name format is:
*
@@ -107,7 +107,8 @@ object Props extends Logger {
* Recognized modes are "development", "test", "profile", "pilot", "staging" and "production"
* with the default run mode being development.
*/
- lazy val mode = {
+ lazy val mode: RunModes.Value = {
+ runModeInitialised = true
Box.legacyNullTest((System.getProperty("run.mode"))).map(_.toLowerCase) match {
case Full("test") => Test
case Full("production") => Production
@@ -115,24 +116,83 @@ object Props extends Logger {
case Full("pilot") => Pilot
case Full("profile") => Profile
case Full("development") => Development
- case _ => {
- val st = Thread.currentThread.getStackTrace
- val names = List(
- "org.apache.maven.surefire.booter.SurefireBooter",
- "sbt.TestRunner",
- "org.specs2.runner.TestInterfaceRunner", // sometimes specs2 runs tests on another thread
- "org.specs2.runner.TestInterfaceConsoleReporter",
- "org.specs2.specification.FragmentExecution"
- )
- if(st.exists(e => names.exists(e.getClassName.startsWith)))
- Test
- else
- Development
+ case _ => (autoDetectRunModeFn.get)()
+ }
+ }
+
+ @volatile private[util] var runModeInitialised: Boolean = false
+
+ /**
+ * Exposes a property affecting run-mode determination, for customisation. If the property is modified
+ * after the run-mode is realised, then it will have no effect and will instead log a warning indicating thus.
+ *
+ * @param name The property name (used to make logging messages clearer, no functional impact).
+ */
+ class RunModeProperty[T](name: String, initialValue: T) extends Logger {
+ @volatile private[this] var value = initialValue
+
+ def get = value
+
+ /**
+ * Attempts to set the property to a new value.
+ *
+ * @return Whether the new property was installed. `false` means modification is no longer allowed.
+ */
+ def set(newValue: T): Boolean =
+ if (allowModification) {
+ value = newValue
+ true
+ } else {
+ onModificationProhibited()
+ false
}
+
+ def allowModification = !runModeInitialised
+
+ def onModificationProhibited() {
+ warn("Setting property " + name + " has no effect. Run mode already initialised to " + mode + ".")
}
}
/**
+ * The default run-mode auto-detection routine uses this function to infer whether Lift is being run in a test.
+ *
+ * This routine can be customised by calling `set` before the run-mode is referenced. (An attempt to customise this
+ * after the run-mode is realised will have no effect and will instead log a warning.)
+ */
+ val doesStackTraceContainKnownTestRunner = new RunModeProperty[Array[StackTraceElement] => Boolean]("doesStackTraceContainKnownTestRunner",
+ (st: Array[StackTraceElement]) => {
+ val names = List(
+ "org.apache.maven.surefire.booter.SurefireBooter",
+ "sbt.TestRunner",
+ "org.jetbrains.plugins.scala.testingSupport.scalaTest.ScalaTestRunner",
+ "org.scalatest.tools.Runner",
+ "org.scalatest.tools.ScalaTestFramework$ScalaTestRunner",
+ "org.scalatools.testing.Runner",
+ "org.scalatools.testing.Runner2",
+ "org.specs2.runner.TestInterfaceRunner", // sometimes specs2 runs tests on another thread
+ "org.specs2.runner.TestInterfaceConsoleReporter",
+ "org.specs2.specification.FragmentExecution"
+ )
+ st.exists(e => names.exists(e.getClassName.startsWith))
+ })
+
+ /**
+ * When the `run.mode` environment variable isn't set or recognised, this function is invoked to determine the
+ * appropriate mode to use.
+ *
+ * This logic can be customised by calling `set` before the run-mode is referenced. (An attempt to customise this
+ * after the run-mode is realised will have no effect and will instead log a warning.)
+ */
+ val autoDetectRunModeFn = new RunModeProperty[() => RunModes.Value]("autoDetectRunModeFn", () => {
+ val st = Thread.currentThread.getStackTrace
+ if ((doesStackTraceContainKnownTestRunner.get)(st))
+ Test
+ else
+ Development
+ })
+
+ /**
* Is the system running in production mode (apply full optimizations)
*/
lazy val productionMode: Boolean = mode == RunModes.Production ||
View
23 core/util/src/test/scala/net/liftweb/util/PropsSpec.scala
@@ -18,17 +18,38 @@ package net.liftweb
package util
import org.specs2.mutable.Specification
-
+import Props.RunModes._
/**
* Systems under specification for Lift Mailer.
*/
object PropsSpec extends Specification {
"Props Specification".title
+ sequential
"Props" should {
"Detect test mode correctly" in {
Props.testMode must_== true
}
+
+ "Allow modification of run-mode properties before the run-mode is set" in {
+ val before = Props.autoDetectRunModeFn.get
+ try {
+ Props.runModeInitialised = false
+ Props.autoDetectRunModeFn.allowModification must_== true
+ Props.autoDetectRunModeFn.set(() => Test) must_== true
+ Props.autoDetectRunModeFn.get must_!= before
+ } finally {
+ Props.autoDetectRunModeFn.set(before)
+ Props.runModeInitialised = true
+ }
+ }
+
+ "Prohibit modification of run-mode properties when the run-mode is set" in {
+ val before = Props.autoDetectRunModeFn.get
+ Props.autoDetectRunModeFn.allowModification must_== false
+ Props.autoDetectRunModeFn.set(() => Test) must_== false
+ Props.autoDetectRunModeFn.get must_== before
+ }
}
}
Something went wrong with that request. Please try again.