Skip to content
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

TestInjector does not respect default value for Boolean flag #373

Closed
komsit37 opened this issue Dec 6, 2016 · 8 comments

Comments

@komsit37
Copy link

commented Dec 6, 2016

When using TestInjector to inject Module with Boolean flag, default value was never used and is always returning true. The default value is correct when injecting in finatra http or thrift server.

I had this issue with finatra 2.4.0, I haven't tried with newer version yet.

Expected behavior

Test pass

Actual behavior

Test fails with 'f.bar was true'

Steps to reproduce the behavior

object BooleanFlagModule extends TwitterModule {
  flag[Boolean]("x", false, "default to false")
}

class Foo @Inject()(@Flag("x") x: Boolean) {
  def bar = x
}

class BooleanFlagTest extends FlatSpec {
  "flag" should "be false" in {
    val inj = TestInjector(BooleanFlagModule)
    val f = inj.instance[Foo]
    assert(!f.bar) //fails: f.bar was true
  }
}
@kevinoliver

This comment has been minimized.

Copy link
Member

commented Dec 7, 2016

i played around with this for a bit, and i suspect the issue is that your TestInjector doesn't get told about the flag.

i put this in to FlagsModuleTest.scala and it passes:

--- a/finatra/inject/inject-app/src/test/scala/com/twitter/inject/app/tests/internal/FlagsModuleTest.scala
+++ b/finatra/inject/inject-app/src/test/scala/com/twitter/inject/app/tests/internal/FlagsModuleTest.scala
@@ -13,6 +13,7 @@ class FlagsModuleTest extends Test {
   val flag = new Flags("FlagsModuleTest", includeGlobal = false, failFastUntilParsed = false)
   val myFlag = flag[String]("my.flag", "This flag has no default")
   val myFlagWithDefault = flag("my.flag.with.default", "default value", "this flag has a default value")
+  private val boolFlag = flag[Boolean]("bool", false, "default to false")
   flag.parseArgs(Array())
   val flagsModule = FlagsModule.create(flag.getAll(includeGlobal = false).toSeq)
 
@@ -30,8 +31,15 @@ class FlagsModuleTest extends Test {
     "use flag without default" in {
       myFlag.get should be(None)
     }
+
+    "see default value for unspecified boolean flags" in {
+      val injector = TestInjector(flagsModule)
+      val theFlag = injector.instance[BoolFlag]
+      assert(!theFlag.value)
+    }
   }
 }

i don't know much more beyond that, but maybe this'll get you going in the right direction.

@komsit37

This comment has been minimized.

Copy link
Author

commented Dec 8, 2016

Hi Kevin, thanks for the direction. I looked into this further and found the issue with TestInjector.

When no flag override is provided, TestInjector try to parse all module flags (see snippet below) (why does it try to do this?). For non-boolean types, this will not change anything, but for boolean type this will override default value to Some(true) (since Flaggable[Boolean] returns Some(true))

snippets of relevant code
TestInjector.scala

  /* Parse module flags with incoming supplied flag values */
    for (moduleFlag <- moduleFlags) {
      flags.get(moduleFlag.name) match {
        case Some(setFlagValue) => moduleFlag.parse(setFlagValue)
        case _ => moduleFlag.parse() //komsit: this resolves to Some(true) for Boolean type
      }
    }

Flag.scala

  /** Parse this flag with no argument. */
  def parse(): Unit = {
    value = flaggable.default
    _parsingDone = true
  }

...
//komsit: default value for Flaggable[Boolean] is Some(true)
  // Pairs of Flaggable conversions for types with corresponding Java boxed types.
  implicit val ofBoolean: Flaggable[Boolean] = new Flaggable[Boolean] {
    override def default = Some(true)
    def parse(s: String) = s.toBoolean
  }

...
//komsit: default value for other Flaggable types is None
abstract class Flaggable[T] {
  /**
   * An optional default value for the Flaggable.
   */
  def default: Option[T] = None
}
@komsit37

This comment has been minimized.

Copy link
Author

commented Dec 8, 2016

One solution I can think of is to leave out flag parsing in the code snippet below (I don't see the reason we need to call moduleFlag.parse() here, it will just set the flag value to whatever is defined in Flaggable[T].default)

    /* Parse module flags with incoming supplied flag values */
    for (moduleFlag <- moduleFlags) {
      flags.get(moduleFlag.name) match {
        case Some(setFlagValue) => moduleFlag.parse(setFlagValue)
        case _ => //moduleFlag.parse() //no need to parse here
      }
    }

flags will be parsed later in InstalledModules.create anyway

    InstalledModules.create(
      flags = moduleFlags,
      modules = modules,
      overrideModules = overrideModules,
      stage = stage).injector

I can verify the fix by adding below test to FlagsModuleTest.scala

    "see default value for boolean flag defined in module" in {
      object BooleanFlagModule extends TwitterModule {
        flag[Boolean]("bool", false, "default to false")
      }
      val injector = TestInjector(BooleanFlagModule)
      injector.instance[BoolFlag].value shouldBe false
    }
@kevinoliver

This comment has been minimized.

Copy link
Member

commented Dec 8, 2016

nice digging and good questions. i don't know the history there so its hard for me to guess as to why its done that way.

i think its worth putting up a PR with the test and fix, and we can sort it out there.

@cacoco

This comment has been minimized.

Copy link
Member

commented Dec 9, 2016

@komsit37 flags need to be resolved (parsed) so that they are injectable. However, I suspect that moduleFlag.parse() is actually incorrect and this should more properly be moduleFlag.get() but I need to dig in further. Like @kevinoliver mentioned, if you think you have a fix and can verify all tests pass, please feel free to put up a PR. Thanks.

@komsit37

This comment has been minimized.

Copy link
Author

commented Dec 12, 2016

Thanks @kevinoliver @cacoco. I will put up a PR later this week

@cacoco

This comment has been minimized.

Copy link
Member

commented Jan 8, 2017

@komsit37 I started looking into this more and have found a couple of issues. The good news is I think I have a fix. Should be out next week. Thanks!

@cacoco cacoco self-assigned this Jan 8, 2017

@cacoco cacoco added the in progress label Jan 8, 2017

@cacoco

This comment has been minimized.

Copy link
Member

commented Jan 11, 2017

@komsit37 a fix for this has landed in 3493778. Please re-open if it doesn't address your issue. Thanks!

@cacoco cacoco closed this Jan 11, 2017

@cacoco cacoco removed the in progress label Jan 11, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.