Migrate majority of partest jvm tests to JUnit #6

gkossakowski · 2013-10-18T15:25:14Z

I'm logging progress on JUnit migration and I wanted to get feedback if this is a good direction.

gkossakowski · 2013-10-18T15:25:44Z

Review by @adriaanm
//cc @retronym

gkossakowski · 2013-10-18T15:43:39Z

Oh and notice that it takes 0.3s to run all of those tests

In partest, we would just manage to initialize single test.

retronym · 2013-10-18T16:16:59Z

LGTM.

You didn't mention the motivation: remove a needless build cycle among our modules.

But even aside from that, I think the move makes sense and might make it easier for people to contribute to the module.

Looking forward a bit, you could express the neg tests with a tiny wrapper around the toolbox compiler as we did for scala-async: https://github.com/scala/async/blob/master/src/test/scala/scala/async/package.scala

dragos · 2013-10-18T16:30:05Z

src/test/scala/scala/xml/MetaDataTest.scala

If you have this annotation for the IDE, it shouldn't be needed.

It was for the IDE. I remember it didn't work before.

In fact, having @RunWith and @Test breaks the junit-interface in SBT 0.13! https://github.com/szeiger/junit-interface/pull/25

@szeiger, I think your PR queue needs some attention! :)

It's only needed if you're using specs or scalatest (without the scalatest plugin).

retronym · 2013-10-18T16:43:35Z

One of the tests is noisy:

[info] Resolving jline#jline;2.11 ...
[info] Done updating.
<hello>world</hello>
<hello>1.5</hello>
<hello>5</hello>
<hello>true</hello>
<hello>5</hello>
<hello>27</hello>
[info] Passed: Total 32, Failed 0, Errors 0, Passed 32

retronym · 2013-10-18T17:39:47Z

Under SBT, the JUnit tests run with an old version of scala-xml:

git diff src/ | egrep --context=5 '\?\?\?|Protection'
+++ b/src/main/scala/scala/xml/Null.scala
@@ -21,6 +21,7 @@ import scala.collection.Iterator
  *  @version 1.0
  */
 case object Null extends MetaData {
+  ???
   override def iterator = Iterator.empty
   override def size = 0
   override def append(m: MetaData, scope: NamespaceBinding = TopScope): MetaData = m
diff --git a/src/test/scala/scala/xml/AttributeTest.scala b/src/test/scala/scala/xml/AttributeTest.scala
index 34c9e4b..13aa7bf 100644
--
--
   @Test
@@ -18,6 +17,7 @@ class MetaDataTest {

   @Test
   def absentElementPrefixed2: Unit = {
+    sys.error(Null.getClass.getProtectionDomain.toString)
     assertEquals(None, Null.get("za://foo.com", TopScope, "bar" ))
     assertEquals(None, Null.get("bar"))
   }
diff --git a/src/test/scala/scala/xml/PrintEmptyElementsTest.scala b/src/test/scala/scala/xml/PrintEmptyElementsTest.scala
index 642829f..f5270b4 100644
pr/6 /code/scala-xml sbt test
[info] Loading global plugins from /Users/jason/.sbt/0.13/plugins
[info] Loading project definition from /Users/jason/code/scala-xml/project
[info] Set current project to scala-xml (in build file:/Users/jason/code/scala-xml/)
[info] Compiling 1 Scala source to /Users/jason/code/scala-xml/target/scala-2.11.0-M5/test-classes...
<hello>world</hello>
<hello>1.5</hello>
<hello>5</hello>
<hello>true</hello>
<hello>5</hello>
<hello>27</hello>
[error] Test scala.xml.MetaDataTest.absentElementPrefixed2 failed: java.lang.RuntimeException: ProtectionDomain  (file:/Users/jason/.ivy2/cache/org.scala-lang.modules/scala-xml_2.11.0-M4/jars/scala-xml_2.11.0-M4-1.0-RC3.jar <no signer certificates>)
[error]  java.net.URLClassLoader@227c808d
[error]  <no principals>
[error]  java.security.Permissions@9f9fec0 (
[error]  (java.io.FilePermission /Users/jason/.ivy2/cache/org.scala-lang.modules/scala-xml_2.11.0-M4/jars/scala-xml_2.11.0-M4-1.0-RC3.jar read)
[error] )
[error]     at scala.sys.package$.error(package.scala:27)
[error]     at scala.xml.MetaDataTest.absentElementPrefixed2(MetaDataTest.scala:20)
[error]     ...
[error] Failed: Total 32, Failed 1, Errors 0, Passed 31
[error] Failed tests:
[error]     scala.xml.MetaDataTest
[error] (test:test) sbt.TestsFailedException: Tests unsuccessful
[error] Total time: 4 s, completed Oct 18, 2013 7:39:30 PM

adriaanm · 2013-10-18T19:02:14Z

Good catch, @retronym! This is why I print the header with full version info/classpath on all partest runs. Maybe we should consider something similar here.

retronym · 2013-10-19T14:17:08Z

Here's how to tweak the ScalaInstance for tests:

def rawScalaInstance = Def.task {
  val prev = scalaInstance.value
  import prev._
  // remove modules from the ScalaInstance used to construct the classloader for tests in `testLoader`
  // otherwise we are not testing the code in this project!
  val filteredExtraJars = extraJars.filterNot(_.getName.contains("xml"))
  ScalaInstance(scalaVersion.value, libraryJar, compilerJar, filteredExtraJars: _*)(makeClassLoader(state.value))
}

scalaInstance in Test <<= rawScalaInstance

This seems to be the least intrusive way to do this. An alternative would be to modify testLoader. Ping @harrah for a sanity check, and few questions: why does all of the ScalaInstance appear on the test classloader by default? Isn't the contents of managed dependencies sufficient? Maybe not so for scalaHome setups?

BTW, I've split it out like that because I need to do the same for another configuration to add code coverage to the build:

harrah · 2013-10-20T21:53:39Z

@retronym Scala classes are cached independently of the managed classpaths in a higher class loader. The purpose is to avoid needing to reload/re-jit the classes, so caching on the exact classpath means you would load the classes in new class loaders if one project needs scala-xml and another doesn't and there would be another class loader for scaladoc, the compiler, etc... Instead, there is one class loader with everything, and then a FilterLoader that ensures that the application only sees classes that come from the classpath.

This could possibly get confused when metadata is not accurate or there are cycles. This should be rare and is probably specific to Scala and its modules. It could also be a bug. You can always fork if necessary.

dragos · 2013-10-21T08:51:15Z

@retronym great coverage report!

gkossakowski · 2013-10-21T10:23:23Z

If we examine the classpath of scala-xml we see:

show test:fullClasspath
[info] List(Attributed(/Users/grek/scala/scala-xml/target/scala-2.11.0-M6/test-classes), 
Attributed(/Users/grek/scala/scala-xml/target/scala-2.11.0-M6/classes), 
Attributed(/Users/grek/.ivy2/cache/org.scala-lang/scala-compiler/jars/scala-compiler-2.11.0-M6.jar), 
Attributed(/Users/grek/.ivy2/cache/org.scala-lang/scala-library/jars/scala-library-2.11.0-M6.jar), 
Attributed(/Users/grek/.ivy2/cache/org.scala-lang.modules/scala-xml_2.11.0-M5/jars/scala-xml_2.11.0-M5-1.0-RC4.jar), 
Attributed(/Users/grek/.ivy2/cache/org.scala-lang.modules/scala-parser-combinators_2.11.0-M5/jars/scala-parser-combinators_2.11.0-M5-1.0-RC2.jar), 
Attributed(/Users/grek/.ivy2/cache/org.scala-lang/scala-reflect/jars/scala-reflect-2.11.0-M6.jar), 
Attributed(/Users/grek/.ivy2/cache/junit/junit/jars/junit-4.11.jar), 
Attributed(/Users/grek/.ivy2/cache/org.hamcrest/hamcrest-core/jars/hamcrest-core-1.3.jar), 
Attributed(/Users/grek/.ivy2/cache/com.novocode/junit-interface/jars/junit-interface-0.10.jar), 
Attributed(/Users/grek/.ivy2/cache/junit/junit-dep/jars/junit-dep-4.10.jar), 
Attributed(/Users/grek/.ivy2/cache/org.scala-tools.testing/test-interface/jars/test-interface-0.5.jar), 
Attributed(/Users/grek/.ivy2/cache/org.scala-lang.modules/scala-partest-interface_2.11.0-M5/jars/scala-partest-interface_2.11.0-M5-0.2.jar), 
Attributed(/Users/grek/.ivy2/cache/org.scala-sbt/test-interface/jars/test-interface-1.0.jar), 
Attributed(/Users/grek/.ivy2/cache/org.scala-lang.modules/scala-partest_2.11.0-M5/jars/scala-partest_2.11.0-M5-1.0.0-RC6.jar), 
Attributed(/Users/grek/.ivy2/cache/org.apache.ant/ant/jars/ant-1.8.4.jar), 
Attributed(/Users/grek/.ivy2/cache/org.apache.ant/ant-launcher/jars/ant-launcher-1.8.4.jar), 
Attributed(/Users/grek/.ivy2/cache/com.googlecode.java-diff-utils/diffutils/jars/diffutils-1.3.0.jar), 
Attributed(/Users/grek/.ivy2/cache/org.scalacheck/scalacheck_2.11.0-M5/jars/scalacheck_2.11.0-M5-1.10.1.jar), 
Attributed(/Users/grek/.ivy2/cache/org.scala-lang/scala-actors/jars/scala-actors-2.11.0-M5.jar))

Note that the order of entries on classpath is correct so it's probably bug in sbt. However, I think the best course of action would be to get rid of scala-xml_2.11.0-M5.jar from test:classpath altogether. I'll look into that.

harrah · 2013-10-21T11:25:46Z

Yeah, duplicate Scala classpath entries is probably a wontfix.

As discovered here: scala#6 (comment) we would have two different versions of scala-xml on test classpath: 1. scala-xml classes compiled from source (we are in scala-xml project) 2. scala-xml transitive dependencies of partest Duplicated classpath entries are always dangerous. I implemented a set of exclude rules that get rid of transitive dependencies of scala-xml. This fixes the problem of JUnit using wrong version of scala-xml for testing (now there's only one choice). Also, this allowed me to to enable conflictWarning again. They are very useful to warn us against dangerous situations that almost always result in broken classpath (as we just learned).

Migrated to JUnit the following tests (from test/files/jvm): * t0632.scala * t1118.scala * unittest_xml.scala None of those tests were dependent on anything jvm specific.

It doesn't seem to test anything XML-specific and I couldn't trace what kind of backend problem this test is supposed to guard against.

gkossakowski · 2013-10-21T14:41:06Z

@retronym: I fixed our classpath in 1d66533. See commit message for details. I believe this is preferred way than low-level tweaking of scalaInstance, although your solution hinted me where to look. Let me know what you think.

gkossakowski · 2013-10-21T21:58:28Z

@dragos: I removed @RunWith annotation and it works fine in Eclipse. Thanks for the tip!

@retronym: I also removed stale println.

As I alluded before, the work-around in 1d66533 is temporary until we figure out what to do with partest dependencies.

This is ready for review.

retronym · 2013-10-21T22:06:05Z

LGTM. Lets book the progress. I confirmed that throwing an exception in the implementation indeed fails the tests. Looking forward to the rest of the transition.

Migrate majority of partest jvm tests to JUnit

adriaanm · 2013-10-22T00:38:45Z

Thanks guys!

dragos reviewed Oct 18, 2013
View reviewed changes

gkossakowski added 8 commits October 21, 2013 16:29

Migrate a few partest jvm tests to JUnit tests.

8b2e8e0

Migrated to JUnit the following tests (from test/files/jvm): * t0632.scala * t1118.scala * unittest_xml.scala None of those tests were dependent on anything jvm specific.

Remove test/files/jvm/backendBugUnapply.scala test.

533afbc

It doesn't seem to test anything XML-specific and I couldn't trace what kind of backend problem this test is supposed to guard against.

Migrate partest jvm/xmlstuff.scala test to JUnit.

952cc8b

Migrate partest jvm/xmlpull.scala test to JUnit.

8ab2d60

Migrate partest jvm/xmlattr.scala test to JUnit.

dcbf212

Migrate partest jvm/xml04embed.scala test to JUnit.

be7046b

Migrate partest jvm/xml03syntax.scala test to JUnit.

e36604e

retronym added a commit that referenced this pull request Oct 21, 2013

Merge pull request #6 from gkossakowski/master

aefc6b9

Migrate majority of partest jvm tests to JUnit

retronym merged commit aefc6b9 into scala:master Oct 21, 2013

Uh oh!

Migrate majority of partest jvm tests to JUnit #6

Migrate majority of partest jvm tests to JUnit #6

Uh oh!

Conversation

gkossakowski commented Oct 18, 2013

Uh oh!

gkossakowski commented Oct 18, 2013

Uh oh!

gkossakowski commented Oct 18, 2013

Uh oh!

retronym commented Oct 18, 2013

Uh oh!

dragos Oct 18, 2013

Choose a reason for hiding this comment

Uh oh!

gkossakowski Oct 18, 2013

Choose a reason for hiding this comment

Uh oh!

retronym Oct 18, 2013

Choose a reason for hiding this comment

Uh oh!

dragos Oct 18, 2013

Choose a reason for hiding this comment

Uh oh!

retronym commented Oct 18, 2013

Uh oh!

retronym commented Oct 18, 2013

Uh oh!

adriaanm commented Oct 18, 2013

Uh oh!

retronym commented Oct 19, 2013

Uh oh!

harrah commented Oct 20, 2013

Uh oh!

dragos commented Oct 21, 2013

Uh oh!

gkossakowski commented Oct 21, 2013

Uh oh!

harrah commented Oct 21, 2013

Uh oh!

gkossakowski commented Oct 21, 2013

Uh oh!

gkossakowski commented Oct 21, 2013

Uh oh!

retronym commented Oct 21, 2013

Uh oh!

adriaanm commented Oct 22, 2013

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants