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

#1259: Simple Test Framework for SBT #1335

Merged
merged 22 commits into from Aug 13, 2019
Merged

#1259: Simple Test Framework for SBT #1335

merged 22 commits into from Aug 13, 2019

Conversation

dkarlinsky
Copy link
Contributor

@dkarlinsky dkarlinsky commented Aug 2, 2019

@dkarlinsky
Copy link
Contributor Author

@jdegoes, I'm kinda stuck trying to get sbt to actually run a dummy RunnableSpec using the new test framework.
I've created a subproject called testRunnerTesting (which is not included in the root project).
It is defined as follows:

lazy val testRunnerTesting = crossProject(JVMPlatform)
  .in(file("test-sbt-testing"))
  .settings(stdSettings("zio-test-sbt-testing"))
  .settings(testFrameworks += new TestFramework("zio.test.runner.ZTestFramework"))
  .dependsOn(testRunner % "test->test;compile->compile")

The sub-project contains a single RunnableSpec object
When I run testRunnerTestingJVM/test in sbt, the output is:

[info] ZIO-Test
[info] Done

Which shows that the test framework is picked up, but the spec is not discovered...

@dkarlinsky
Copy link
Contributor Author

Ok, I think I figured it out.
It looks like sbt doesn't support parametrized types in SubclassFingerprint.superclassName.
Once I switched to DefaultRunnableSpec discovery started working

@jdegoes
Copy link
Member

jdegoes commented Aug 4, 2019

@dkarlinsky Could it be name mangling or some other thing that's causing the problem?

DefaultRunnableSpec is only for a common case when the Runner uses the ZIO default environment.


sealed abstract class ExecutedSpecStructure {
def stats: Stats
def traverse[R](
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to reinvent foldM, will look more further down.

}
}

sealed abstract class ZTestResult(val stats: Stats) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can reuse Assertion here. It has the same structure (you are using constants for Ignore and Success).

Otherwise we have 2 data structures with a similar hierarchy (success, ignore, failure w/value).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes this makes sense now.

@dkarlinsky
Copy link
Contributor Author

dkarlinsky commented Aug 4, 2019

@jdegoes don't think so.
I managed to step through the sbt discovery code using a debugger.
At this line c.structure.parents, for a DefaultRunnableSpec looks as follows:

0 = {Projection@18173} "Projection(prefix: Singleton(path: Path(components: [Lxsbti.api.PathComponent;@6741c768)), id: DefaultRunnableSpec)"
1 = {Parameterized@18461} "Parameterized(baseType: Projection(prefix: Singleton(path: Path(components: [Lxsbti.api.PathComponent;@1659053e)), id: RunnableSpec), typeArguments: [Lxsbti.api.Type;@53749d12)"
2 = {Projection@18462} "Projection(prefix: Singleton(path: Path(components: [Lxsbti.api.PathComponent;@28d4e9be)), id: Object)"
3 = {Projection@18463} "Projection(prefix: Singleton(path: Path(components: [Lxsbti.api.PathComponent;@41c7114b)), id: Any)"

As you can see 1 is the sbt's internal representation of RunnableSpec parent and it is type is Parameterized.
But next thing that happens, is that the parent types are flat mapped with simpleName method, which doesn't have a case for Parameterized, returning None.
So RunnableSpec parent is never matched against the SubclassFingerprit.superclassName declared by the test framework...

object Reflect {
type EnableReflectiveInstantiation =
org.portablescala.reflect.annotation.EnableReflectiveInstantiation
}
Copy link
Member

@jdegoes jdegoes Aug 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

N/A


/**
* A `RunnableSpec` has a main function and can be run by the JVM / Scala.js.
*/
@EnableReflectiveInstantiation
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes ZIO test have a dependency on portable reflection library. Hmm, we have to think if it's worth it or not. Does it support Dotty?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it might not...
It says: Scala 2.10.x, 2.11.x, 2.12.x and 2.13.0-M4 on their readme.
In any case it looks like I'm still missing some pieces to have sbt run specs under Scala.js

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scalatest recently ported & published for dotty, might want to look at what they do. (Also, dotty works just fine with 2.12 test frameworks under DottyCompat mode, might not need any special support)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, let's leave it in for now, we can decide for sure later! Maybe some small Scala.js code is all we need to do the reflection (iterate keys in global / window).

@jdegoes
Copy link
Member

jdegoes commented Aug 4, 2019

I managed to step through the sbt discovery code using a debugger.
At this line c.structure.parents, for a DefaultRunnableSpec looks as follows:

@dkarlinsky Great detective work!

I think this is a bug in SBT. However, we can work around it in ZIO test.

We can change RunnerSpec to use type members instead of type variables, something like:

abstract class RunnableSpec {
  type Label
  type Test

  def runner: TestRunner[Label, Test]
  def spec = Spec[Label, Test]

  /**
   * A simple main function that can be used to run the spec.
   *
   * TODO: Parse command line options.
   */
  final def main(args: Array[String]): Unit = { val _ = runner.unsafeRunSync(spec) }

  /**
   * Returns an effect that executes the spec, producing the results of the execution.
   */
  final val run: UIO[ExecutedSpec[Label]] = runner.run(spec)
}
object RunnableSpec {
  type Aux[L, T] = RunnableSpec { type Label = L; type Test = T }

  def apply[L, T](runner0: TestRunner[L, T])(spec0: => Spec[Label, Test]): RunnableSpec = 
    new RunnableSpec {
      type Label = L 
      type Test = T

      def runner = runner0
      def spec = spec0
    }
}

Now RunnerSpec can be used as before, but it has no type parameters.

What do you think?

@dkarlinsky
Copy link
Contributor Author

object RunnableSpec {
  type Aux[L, T] = RunnableSpec { type Label = L; type Test = T }

  def apply[L, T](runner0: TestRunner[L, T])(spec0: => Spec[Label, Test]): RunnableSpec = 
    new RunnableSpec {
      type Label = L 
      type Test = T

      def runner = runner0
      def spec = spec0
    }
}

This looks very nice as a constructor for a new RunnableSpec, but I don't see how this will work with subclassing.
The specs picked up by sbt need to be objects that are subclasses of RunnableSpec...

@@ -0,0 +1,14 @@
package zio.test.runner
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd add this to the top-level examples project that depends on both ZIO Core and ZIO Tests. It can have examples of both ZIO Test and ZIO proper.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea for this project was to be able to manually run testRunnerTestingJVM/test to test the sbt runner.
I guess I could to the same on the examples project...

@jdegoes
Copy link
Member

jdegoes commented Aug 5, 2019

This looks very nice as a constructor for a new RunnableSpec, but I don't see how this will work with subclassing.

Hmm, can you try this:

abstract class AbstractRunnableSpec { ... } // As before
type RunnableSpec[L, T] = AbstractRunnableSpec { type Label = L; type Test = T }

Then perhaps the user can do:

class MyRunnableSpec extends RunnableSpec(MyRunner) { 
  ...
}

If we have the appropriate 2 parameter list constructor.

@dkarlinsky
Copy link
Contributor Author

This looks very nice as a constructor for a new RunnableSpec, but I don't see how this will work with subclassing.

Hmm, can you try this:

abstract class AbstractRunnableSpec { ... } // As before
type RunnableSpec[L, T] = AbstractRunnableSpec { type Label = L; type Test = T }

Then perhaps the user can do:

class MyRunnableSpec extends RunnableSpec(MyRunner) { 
  ...
}

If we have the appropriate 2 parameter list constructor.

I don't think AbstractRunnableSpec can have a constructor that accepts Spec and Runner because we don't have the Label and Test types defined yet...
Currently, I just have DefaultRunnableSpec define the abstract types and the two parameter values directly.
Perhaps we can have AbstractRunnableSpec extending RunnableSpec to make it easier to implement specs with non default runner. Something like:

abstract class AbstractRunnableSpec[L, T](runner0: TestRunner[L, T])(spec0: => Spec[L, T]) extends RunnableSpec {
  override type Label = L
  override type Test = T

  override def runner = runner0
  override def spec =spec0
}

Then DefaultRunnableSpec can go back to looking like it did before.

@neko-kai
Copy link
Member

neko-kai commented Aug 5, 2019

@dkarlinsky Extending it should work; We use a lot of classes with type parameters that extend base Scalatest classes and they're discovered as expected. I'd expect the bug with type parameter handling to apply only to the class that you're searching for, not to subclasses. (Coincidentally, using defs in AbstractRunnableSpec just makes it easier for me to plug a mutable DSL if I decide to 😆 )

@jdegoes
Copy link
Member

jdegoes commented Aug 6, 2019

@dkarlinsky

This is a good idea.

So let's define the following:

abstract class AbstractRunnableSpec {
  type Label
  type Test

  def runner: TestRunner[Label, Test]
  def spec: Spec[Label, Test])

  ...
}
abstract class RunnableSpec[L, T](runner: TestRunner[L, T])(spec0: => Spec[L, T]) extends AbstractRunnableSpec[L, T] {
  type Label = L
  type Test = T

  def spec = spec0
}

Now we have the same syntax as before, just with a workaround for the bug.

Sound good?

/cc @Kaishh

@dkarlinsky
Copy link
Contributor Author

@jdegoes I think I'm done with all the functionality and requested changes.
SBT runs tests under both JVM and JS

Copy link
Member

@neko-kai neko-kai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic!

@jdegoes
Copy link
Member

jdegoes commented Aug 13, 2019

@dkarlinsky Awesome work! ZIO Test is one more step closer to fully operational...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants