-
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 18 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( | ||
|
@@ -494,6 +511,55 @@ 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( | ||
inputs=[archive], | ||
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="unzip -l {archive} | grep {combined_patterns} > {out}".format(archive=archive.path, combined_patterns=_prep_grep_pattern_from(patterns), out=discovered_classes.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. can we use 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, I actually thought about it but decided (for some reason I don't remember) on unzip. I'll change it. 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 moved to jar but when I tried to use the one from bazel_tools I got the following error:
My attempt was via adding an internal attribute (like _ijar), add it to the inputs of the action and then use it instead of the vanilla jar command but that doesn't seem like it's enough. It seems more stuff are needed as inputs. Any idea? |
||
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) | ||
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 we wrap this line on return struct(
suite_class = "foo",
classesFlag = "bar",
... |
||
|
||
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 += [ | ||
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 we move the next line up to start here: |
||
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 +689,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(): | ||
|
@@ -728,4 +818,24 @@ 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={ | ||
"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")), | ||
} + _implicit_deps + _common_attrs, | ||
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,51 @@ | ||
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", | ||
"@scala//:scala-xml", | ||
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. does this really need xml and parser combinators? If so, that's a bummer. 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. it does. why is that a bummer? 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. xml literals are deprecated, and the library is being removed from the standard library, I think. |
||
"@scala//:scala-parser-combinators", | ||
"@scala//:scala-library", | ||
"@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,45 @@ | ||
package io.bazel.rulesscala.test_discovery | ||
|
||
import org.junit.runner.RunWith | ||
import org.junit.runner.Runner | ||
import org.junit.runners.Suite | ||
import org.junit.runners.model.RunnerBuilder | ||
|
||
@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(filterWhitespace) | ||
.map(filterMetadata) | ||
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. strange spacing here. 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. true, sublime editing. Will fix. |
||
.map(dropFileSuffix) | ||
.map(fileToClassFormat) | ||
.map(Class.forName) | ||
|
||
private def filterMetadata(zipEntryParts: Array[String]): String = | ||
zipEntryParts.last | ||
|
||
private def filterWhitespace(zipEntry: String): Array[String] = | ||
zipEntry.split("\\s+") | ||
|
||
private def dropFileSuffix(classEntry: String): String = | ||
classEntry.split("\\.").head | ||
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. inconsistent indentation in this file. 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. true, sublime editing. Will fix. |
||
|
||
//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") | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
package scala.test.junit | ||
|
||
import org.junit.Test | ||
import org.junit.runner.RunWith | ||
import org.junit.runners.JUnit4 | ||
import org.junit.Assert._ | ||
|
||
class TestJunitCustomPattern { | ||
|
||
@Test | ||
def someTest: Unit = { | ||
assertEquals(1, 1) | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
package scala.test.junit | ||
|
||
import org.junit.Test | ||
import org.junit.runner.RunWith | ||
import org.junit.runners.JUnit4 | ||
import org.junit.Assert._ | ||
|
||
class JunitCustomSuffixE2E { | ||
|
||
@Test | ||
def someTest: Unit = { | ||
assertEquals(1, 1) | ||
} | ||
|
||
} |
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