-
Notifications
You must be signed in to change notification settings - Fork 281
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
Junit & Specs2 support #163
Changes from 29 commits
7e2e198
34620c8
41c0a48
60caa69
410018d
287921f
6fd76c9
87dbf01
ea10b40
9effdb9
f9f084c
3158e76
0033f21
d0e7089
5c57a83
5161db1
99a639c
eb7095f
30fef56
64a6404
97583a1
558640d
d52045a
c228907
0f6cd99
9dd6e56
3524c4e
5572178
66f5eb3
b8faa37
4cc722d
853679a
bb23f05
0325608
fe20920
22ccd22
7d1a915
845fb2f
39824a7
d71bc1d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
def junit_repositories(): | ||
native.maven_jar( | ||
name = "io_bazel_rules_scala_junit_junit", | ||
artifact = "junit:junit:4.12", | ||
sha1 = "2973d150c0dc1fefe998f834810d68f278ea58ec", | ||
) | ||
native.bind(name = 'io_bazel_rules_scala/dependency/junit/junit', actual = '@io_bazel_rules_scala_junit_junit//jar') | ||
|
||
native.maven_jar( | ||
name = "io_bazel_rules_scala_org_hamcrest_hamcrest_core", | ||
artifact = "org.hamcrest:hamcrest-core:1.3", | ||
sha1 = "42a25dc3219429f0e5d060061f71acb49bf010a0", | ||
) | ||
native.bind(name = 'io_bazel_rules_scala/dependency/hamcrest/hamcrest_core', actual = '@io_bazel_rules_scala_org_hamcrest_hamcrest_core//jar') |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,7 @@ | |
|
||
"""Rules for supporting the Scala language.""" | ||
|
||
load("//specs2:specs2_junit.bzl", "specs2_junit_dependencies") | ||
_jar_filetype = FileType([".jar"]) | ||
_java_filetype = FileType([".java"]) | ||
_scala_filetype = FileType([".scala"]) | ||
|
@@ -298,6 +299,22 @@ def _write_launcher(ctx, jars): | |
content=content) | ||
|
||
|
||
def _write_junit_test_launcher(ctx, jars, test_suite): | ||
content = """#!/bin/bash | ||
{java} -cp {cp} -ea {classesFlag} {jvm_flags} org.junit.runner.JUnitCore {test_suite_class} | ||
""" | ||
content = content.format( | ||
java=ctx.file._java.short_path, | ||
cp=":".join([j.short_path for j in jars]), | ||
test_suite_class=test_suite.suite_class, | ||
classesFlag = test_suite.classesFlag, | ||
#allows setting xmx for example for tests which use a lot of memory | ||
jvm_flags = " ".join(ctx.attr.jvm_flags) | ||
) | ||
ctx.file_action( | ||
output=ctx.outputs.executable, | ||
content=content) | ||
|
||
def _write_test_launcher(ctx, jars): | ||
if len(ctx.attr.suites) != 0: | ||
print( | ||
|
@@ -483,8 +500,7 @@ def _scala_test_impl(ctx): | |
jars = _collect_jars(deps) | ||
(cjars, rjars) = (jars.compiletime, jars.runtime) | ||
cjars += [ctx.file._scalareflect, ctx.file._scalatest, ctx.file._scalaxml] | ||
rjars += [ | ||
ctx.outputs.jar, | ||
rjars += [ctx.outputs.jar, | ||
ctx.file._scalalib, | ||
ctx.file._scalareflect, | ||
ctx.file._scalatest, | ||
|
@@ -494,6 +510,58 @@ def _scala_test_impl(ctx): | |
_write_test_launcher(ctx, rjars) | ||
return _scala_binary_common(ctx, cjars, rjars) | ||
|
||
def _prep_grep_pattern_from(patterns): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you add a comment on the motivation of patterns? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added in the general comment about how the design works. Do you want another comment here? |
||
combined_pattern = "" | ||
for pattern in patterns: | ||
combined_pattern += " -e {}\.class".format(pattern) | ||
return combined_pattern | ||
|
||
def _discover_classes(ctx, patterns, archive): | ||
discovered_classes = ctx.new_file(ctx.label.name + "_discovered_classes.txt") | ||
ctx.action( | ||
#We need _jdk to even run _jar. Depending on _jar is not enough with sandbox | ||
inputs= [archive] + ctx.files._jar + ctx.files._jdk, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess this is supposed to be then I think we don't need There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TL;DR: Definitely prefer ctx.executable._jar here, especially since you are using it as an executable. So, I did some major experimentation with this. _jar being marked as executable should be sufficient to automatically pull in its runfiles regardless of whether you access via executable or files. (ctx.action WILL NOT find runfiles on all inputs, just those marked with executable=True. Note that you can still find runfiles on any label, executable or not, via ctx.attr.....data_runfiles|default_runfiles.files. That said, I think it is safest to access via ctx.executable._jar so it is clear that it is being used as an executable and that there is an error if you accidentally remove executable=True. |
||
outputs=[discovered_classes], | ||
progress_message="Discovering classes with patterns of %s" % patterns, | ||
#TODO consider with Damien/Ulf/Oscar the implications of switching from grep to scala code | ||
#Pro-> logic will be cohesive (currently the scala code assumes stuff from the grep) | ||
#Con-> IIRC Ulf warned me about performance implications of these traversals | ||
command="{jar} -tf {archive} | grep {combined_patterns} > {out}".format(archive=archive.path, combined_patterns=_prep_grep_pattern_from(patterns), out=discovered_classes.path, | ||
jar=ctx.file._jar.path)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1, since merely calling it to execute it here and I am targeting removing single_file=True in other PR. |
||
return discovered_classes | ||
|
||
def _gen_test_suite_based_on_prefix(ctx, archive): | ||
discovered_classes = _discover_classes(ctx, ctx.attr.patterns, archive) | ||
return struct(suite_class = "io.bazel.rulesscala.test_discovery.DiscoveredTestSuite", | ||
classesFlag = "-Dbazel.discovered.classes.file.path=%s" % discovered_classes.short_path, | ||
discovered_classes = discovered_classes) | ||
|
||
def _scala_junit_test_impl(ctx): | ||
deps = ctx.attr.deps + [ctx.attr._suite] | ||
jars = _collect_jars(deps) | ||
(cjars, rjars) = (jars.compiletime, jars.runtime) | ||
junit_deps = [ctx.file._junit,ctx.file._hamcrest] | ||
cjars += junit_deps | ||
rjars += [ctx.outputs.jar, | ||
ctx.file._scalalib | ||
] + junit_deps | ||
rjars += _collect_jars(ctx.attr.runtime_deps).runtime | ||
test_suite = _gen_test_suite_based_on_prefix(ctx, ctx.outputs.jar) | ||
_write_junit_test_launcher(ctx, rjars, test_suite) | ||
|
||
common_binary = _scala_binary_common(ctx, cjars, rjars) | ||
discovered_classes_runfiles = ctx.runfiles( | ||
files = [test_suite.discovered_classes]) | ||
|
||
#TODO is there a way to easily merge both structs | ||
#I'd like a scala case class copy: | ||
#common_binary.copy(runfiles= common_binary.runfiles.merge(discovered_classes_runfiles)) | ||
return struct( | ||
files = common_binary.files, | ||
scala = common_binary.scala, | ||
runfiles= common_binary.runfiles.merge(discovered_classes_runfiles)) | ||
|
||
|
||
_implicit_deps = { | ||
"_ijar": attr.label(executable=True, cfg="host", default=Label("@bazel_tools//tools/jdk:ijar"), single_file=True, allow_files=True), | ||
"_scalac": attr.label(executable=True, cfg="host", default=Label("//src/java/io/bazel/rulesscala/scalac"), allow_files=True), | ||
|
@@ -623,6 +691,30 @@ exports_files([ | |
"lib/scala-xml_2.11-1.0.4.jar", | ||
"lib/scalap-2.11.8.jar", | ||
]) | ||
|
||
filegroup( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we already have these here: https://github.com/bazelbuild/rules_scala/blob/master/src/scala/BUILD#L1 Can we consolidate on those targets? Why were these needed? I'm concerned about supporting 2.12 and 2.11 with all these direct references in different places. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we can, they were needed since using "@scala//:lib/scala-reflect.jar" as a label didn't work for me. If you know how I can skip that I'd love to know. I know my PR notes were long but this is one of the things I noted there. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. :( Out of luck.
Any idea why I'm getting this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. isn't the target There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. any comment about this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, moved to
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @johnynek |
||
name = "scala-xml", | ||
srcs = ["lib/scala-xml_2.11-1.0.4.jar"], | ||
visibility = ["//visibility:public"], | ||
) | ||
|
||
filegroup( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why are these needed at all rather than |
||
name = "scala-parser-combinators", | ||
srcs = ["lib/scala-parser-combinators_2.11-1.0.4.jar"], | ||
visibility = ["//visibility:public"], | ||
) | ||
|
||
filegroup( | ||
name = "scala-library", | ||
srcs = ["lib/scala-library.jar"], | ||
visibility = ["//visibility:public"], | ||
) | ||
|
||
filegroup( | ||
name = "scala-reflect", | ||
srcs = ["lib/scala-reflect.jar"], | ||
visibility = ["//visibility:public"], | ||
) | ||
""" | ||
|
||
def scala_repositories(): | ||
|
@@ -658,6 +750,14 @@ def scala_repositories(): | |
server = "scalac_deps_maven_server", | ||
) | ||
|
||
native.bind(name = 'io_bazel_rules_scala/dependency/scala/scala_xml', actual = '@scala//:scala-xml') | ||
|
||
native.bind(name = 'io_bazel_rules_scala/dependency/scala/parser_combinators', actual = '@scala//:scala-parser-combinators') | ||
|
||
native.bind(name = 'io_bazel_rules_scala/dependency/scala/scala_library', actual = '@scala//:scala-library') | ||
|
||
native.bind(name = 'io_bazel_rules_scala/dependency/scala/scala_reflect', actual = '@scala//:scala-reflect') | ||
|
||
def scala_export_to_java(name, exports, runtime_deps): | ||
jars = [] | ||
for target in exports: | ||
|
@@ -728,4 +828,25 @@ def scala_library_suite(name, | |
ts.append(n) | ||
scala_library(name = name, deps = ts, exports = exports + ts, visibility = visibility) | ||
|
||
scala_junit_test = rule( | ||
implementation=_scala_junit_test_impl, | ||
attrs= _implicit_deps + _common_attrs + { | ||
"patterns": attr.string_list(default=["Test"]), | ||
"_junit": attr.label(default=Label("//external:io_bazel_rules_scala/dependency/junit/junit"), single_file=True), | ||
"_hamcrest": attr.label(default=Label("//external:io_bazel_rules_scala/dependency/hamcrest/hamcrest_core"), single_file=True), | ||
"_suite": attr.label(default=Label("//src/java/io/bazel/rulesscala/test_discovery:test_discovery")), | ||
"_jar": attr.label(executable=True, cfg="host", default=Label("@bazel_tools//tools/jdk:jar"), single_file=True, allow_files=True), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. single_file=True should not be necessary here. Also, this should already be in _implicit_deps. |
||
}, | ||
outputs={ | ||
"jar": "%{name}.jar", | ||
"deploy_jar": "%{name}_deploy.jar", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we need to make a deploy_jar for a test? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because of the code reuse of |
||
"manifest": "%{name}_MANIFEST.MF", | ||
}, | ||
test=True, | ||
) | ||
|
||
def scala_specs2_junit_test(name, **kwargs): | ||
scala_junit_test( | ||
name = name, | ||
deps = specs2_junit_dependencies() + kwargs.pop("deps",[]), | ||
**kwargs) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
def specs2_version(): | ||
return "3.8.8" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we need this special function? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. answered in the other comment about it |
||
def specs2_repositories(): | ||
|
||
native.maven_jar( | ||
name = "io_bazel_rules_scala_org_specs2_specs2_core", | ||
artifact = "org.specs2:specs2-core_2.11:" + specs2_version(), | ||
sha1 = "495bed00c73483f4f5f43945fde63c615d03e637", | ||
) | ||
native.bind(name = 'io_bazel_rules_scala/dependency/specs2/specs2_core', actual = '@io_bazel_rules_scala_org_specs2_specs2_core//jar') | ||
|
||
native.maven_jar( | ||
name = "io_bazel_rules_scala_org_specs2_specs2_common", | ||
artifact = "org.specs2:specs2-common_2.11:" + specs2_version(), | ||
sha1 = "15bc009eaae3a574796c0f558d8696b57ae903c3", | ||
) | ||
native.bind(name = 'io_bazel_rules_scala/dependency/specs2/specs2_common', actual = '@io_bazel_rules_scala_org_specs2_specs2_common//jar') | ||
|
||
native.maven_jar( | ||
name = "io_bazel_rules_scala_org_specs2_specs2_matcher", | ||
artifact = "org.specs2:specs2-matcher_2.11:" + specs2_version(), | ||
sha1 = "d2e967737abef7421e47b8994a8c92784e624d62", | ||
) | ||
native.bind(name = 'io_bazel_rules_scala/dependency/specs2/specs2_matcher', actual = '@io_bazel_rules_scala_org_specs2_specs2_matcher//jar') | ||
|
||
native.maven_jar( | ||
name = "io_bazel_rules_scala_org_scalaz_scalaz_effect", | ||
artifact = "org.scalaz:scalaz-effect_2.11:7.2.7", | ||
sha1 = "824bbb83da12224b3537c354c51eb3da72c435b5", | ||
) | ||
native.bind(name = 'io_bazel_rules_scala/dependency/scalaz/scalaz_effect', actual = '@io_bazel_rules_scala_org_scalaz_scalaz_effect//jar') | ||
|
||
native.maven_jar( | ||
name = "io_bazel_rules_scala_org_scalaz_scalaz_core", | ||
artifact = "org.scalaz:scalaz-core_2.11:7.2.7", | ||
sha1 = "ebf85118d0bf4ce18acebf1d8475ee7deb7f19f1", | ||
) | ||
native.bind(name = 'io_bazel_rules_scala/dependency/scalaz/scalaz_core', actual = '@io_bazel_rules_scala_org_scalaz_scalaz_core//jar') | ||
|
||
def specs2_dependencies(): | ||
return [ | ||
"//external:io_bazel_rules_scala/dependency/specs2/specs2_core", | ||
"//external:io_bazel_rules_scala/dependency/specs2/specs2_common", | ||
"//external:io_bazel_rules_scala/dependency/specs2/specs2_matcher", | ||
"//external:io_bazel_rules_scala/dependency/scalaz/scalaz_effect", | ||
"//external:io_bazel_rules_scala/dependency/scalaz/scalaz_core", | ||
"//external:io_bazel_rules_scala/dependency/scala/scala_xml", | ||
"//external:io_bazel_rules_scala/dependency/scala/parser_combinators", | ||
"//external:io_bazel_rules_scala/dependency/scala/scala_library", | ||
"//external:io_bazel_rules_scala/dependency/scala/scala_reflect"] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
load("//specs2:specs2.bzl", "specs2_repositories", "specs2_dependencies", "specs2_version") | ||
load("//junit:junit.bzl", "junit_repositories") | ||
|
||
def specs2_junit_repositories(): | ||
specs2_repositories() | ||
junit_repositories() | ||
# Aditional dependencies for specs2 junit runner | ||
native.maven_jar( | ||
name = "io_bazel_rules_scala_org_specs2_specs2_junit_2_11", | ||
artifact = "org.specs2:specs2-junit_2.11:" + specs2_version(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see how that function helps (specs2_version) since only one version will have the right sha1 below, no (unless someone generates a sha1 collision for specs2! ;) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As I mentioned in the PR notes the reason I added this was because I (mistakenly) used one version in the specs2.bzl file and a different one in the specs2_junit.bzl file. Finding that bug took me a few hours since the runtime error messages are really unclear. Once I found it out I realized my problem was code duplication so I solved it with a method. |
||
sha1 = "1dc9e43970557c308ee313842d84094bc6c1c1b5", | ||
) | ||
native.bind(name = 'io_bazel_rules_scala/dependency/specs2/specs2_junit', actual = '@io_bazel_rules_scala_org_specs2_specs2_junit_2_11//jar') | ||
|
||
def specs2_junit_dependencies(): | ||
return specs2_dependencies() + ["//external:io_bazel_rules_scala/dependency/specs2/specs2_junit"] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
load("//scala:scala.bzl", "scala_library") | ||
|
||
|
||
scala_library(name = "test_discovery", | ||
srcs = ["DiscoveredTestSuite.scala"], | ||
deps = ["//external:io_bazel_rules_scala/dependency/junit/junit"], | ||
visibility = ["//visibility:public"], | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
package io.bazel.rulesscala.test_discovery | ||
|
||
import org.junit.runner.RunWith | ||
import org.junit.runners.Suite | ||
import org.junit.runners.model.RunnerBuilder | ||
/* | ||
The test running and discovery mechanism works in the following manner: | ||
- Bazel rule executes a JVM application to run tests (currently `JUnitCore`) and asks it to run | ||
the `DiscoveredTestSuite` suite. | ||
- When JUnit tries to run it, it uses the `TextFileSuite` runner | ||
to know what tests exist in the suite. | ||
- We know which tests to run by examining the entries of the target's archive. | ||
- The entries of the archive are filtered with grep in the bazel rule using the defined patterns. | ||
- The matching entries are written to a file | ||
- It's path is passed in a system property ("bazel.discovered.classes.file.path"). | ||
- We iterate over the entries and format them into classes. | ||
- At this point we tell JUnit (via the `RunnerBuilder`) what are the discovered test classes. | ||
- W.R.T. discovery semantics this is very similar to how maven surefire/failsafe plugins work. | ||
|
||
Additional references: | ||
- http://junit.org/junit4/javadoc/4.12/org/junit/runner/RunWith.html | ||
- http://junit.org/junit4/javadoc/4.12/org/junit/runners/model/RunnerBuilder.html | ||
- http://maven.apache.org/surefire/maven-surefire-plugin/examples/inclusion-exclusion.html | ||
*/ | ||
@RunWith(classOf[TextFileSuite]) | ||
class DiscoveredTestSuite | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you add a comment to the approach here? It looks like this is always registered at the test to run, and then by environmental variable all classes are passed in. Is this right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that's right. I'll add a comment though I'd rather try and change the name to make it more explicit. If it was called There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think documenting the architecture of how you are doing this is a good idea. To be honest, I'm only probably 80% aware how, and since I won't be using this code, in 6 months I'll probably be at 10% and may have to review patches at that time. I'd really appreciate a paragraph here explaining the approach rather than someone having to go read about junit architecture, then put together the approach from integrating the .bzl and scala code. |
||
|
||
class TextFileSuite(testClass: Class[Any], builder: RunnerBuilder) | ||
extends Suite(builder, testClass, TextFileSuite.discoveredClasses) | ||
|
||
object TextFileSuite { | ||
|
||
private val discoveredClasses = readDiscoveredClasses(classesRegistry) | ||
|
||
private def readDiscoveredClasses(classesRegistry: String): Array[Class[_]] = | ||
entries(classesRegistry) | ||
.map(dropFileSuffix) | ||
.map(fileToClassFormat) | ||
.map(Class.forName) | ||
|
||
private def dropFileSuffix(classEntry: String): String = | ||
classEntry.split("\\.").head | ||
|
||
//name is too imperative. Not sure how to change to declarative | ||
private def fileToClassFormat(classEntry: String): String = | ||
classEntry.replace('/', '.') | ||
|
||
private def entries(classesRegistry: String): Array[String] = | ||
scala.io.Source.fromFile(classesRegistry).getLines.toArray | ||
|
||
private def classesRegistry: String = | ||
System.getProperty("bazel.discovered.classes.file.path") | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Depending on PR submission order one of us will have to remember this) this will be the call to the unified _write_launcher in #171:
Edit: change name of parameter to launcherJvmFlags