-
Notifications
You must be signed in to change notification settings - Fork 74
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
Add convenience feature to TDML Runner #1375
base: main
Are you sure you want to change the base?
Conversation
project/Dependencies.scala
Outdated
@@ -53,7 +53,8 @@ object Dependencies { | |||
lazy val test = Seq( | |||
"junit" % "junit" % "4.13.2" % "test", | |||
"com.github.sbt" % "junit-interface" % "0.13.3" % "test", | |||
"org.scalacheck" %% "scalacheck" % "1.18.1" % "test" | |||
"org.scalacheck" %% "scalacheck" % "1.18.1" % "test", | |||
"com.lihaoyi" %% "sourcecode" % "0.4.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.
Should this be a "test" dependency? I think we've also been sorting these alphabetically.
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.
It didn't work when I had it only as a test dependency. Our TDML Runner is "main" code not "test" code.
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.
If this is main code then does it get pulled into the CLI helpers? If so we need to list the dependency the CLI LICENSE/NOTICE file since it's no ALv2.
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.
To clarify, to us the tdml-lib library is "main" code. Our users will incorporate that dependency as a "test" dependency.
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.
This should not be added to 'val test'. test
dependencies are added to all jars. This wants to only be a dependency of daffodil-tdml. We probably want a special val tdml = Seq(...sourcecode)
, which is added to the project.
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.
Yeah, I think we need another definition in Dependencies.scala for use with just tdml specific dependencies, and it will have just this one.
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.
@stevedlawrence This is just a dependency for the tdml-lib now.
Li Haoyi's library is MIT license, which is Apache compatible.
While the CLI does run TDML tests, and so depends on daffodil-tdml-lib, it isn't dependent on this feature, which is only used by Scala code that explicitly creates calls to do the Runner.doTest method, which depends on this sourcecode macro which is passed through to scala code users in the implicit argument to doTest.
But should we add info about this library anyway just to be safe? I am not sure CLI won't pull it in. I imagine it will just depend on everything that daffodil-tdml-lib depends on?
This would mean adding to the CLI's bin.LICENSE file I believe.
Is there anyplace else to update on account of this dependency?
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.
If sourcecode
is a transitive dependency of the CLI via daffdodil-tdml, then we must include license/notice information in daffodil-cli/bin.{LICENSE,NOTICE)
. All CLI transitive dependencies are packaged/distributed in our CLI helper binaries whether they are used or not, and anything packaged/distributed in there needs to be mentioned in the the CLI specific license files. Transitive dependencies do not need to be listed anywhere else since we don't distribute them, as long they are ASF compatible or optional they are fine.
Note: See my other thought about whether this really wants to be part of daffodil-sbt. It's pretty easy to add new dependencies via daffodil-sbt.
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.
The more I think about this, I don't think this wants to be a source dependency. It really just wants to be a "test" dependency for use by JUnit files. If we want to also use it for schema projects, then we should modify daffodil-sbt to also add it as a "test" dependency. That avoids the awkwardness of distributing something with the CLI that is never used by the CLI. There's a bit of duplication, but it makes it so we can use the sourcecode
approach (or the alternative junit approach) with any version of Daffodil for schema projects. Otherwise to test with an older daffodilVersion
you would need to revert runner.doTest
since sourcecode
would only be pulled in for newer versions of daffodil-lib. The fix is to then manually add sourcecode
dependency for those projects, but that's a pain when we could just add support in daffodil-sbt.
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.
Yeah, since the daffodil-sbt plugin takes care of the dependency on daffodil-tdml-processor, it has to add the "test" qualification now.
A daffodil-tdml-junit library is sounding more appealing even without daffodil-sbt support for it.
In fact our daffodil libraries really do need to be usable without daffodil-sbt. E.g., suppose someone's corporate standard is maven or other tool. We need all the daffodil libraries including the tdml runner, etc. all to be usable without depending on daffodil-sbt. Having a isolated library where all the junit dependencies for gluing together tdml and junit testing makes sense.
daffodil-tdml-lib/src/main/scala/org/apache/daffodil/tdml/RunnerFactory.scala
Show resolved
Hide resolved
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.
+1 just a note about versions!
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.
+1
I don't have a strong opinion on using this library vs rolling our own. One advantage I see is that if we use the library it might be compatible with newer versions of Scala whereas we may need to re-write our hand written one when we update versions, but like Steve said it would probably be a very simple macro anyway.
This lets you use the name of the test method implicitly as the name of the TDML test it will run. For example @test def testCaseName(): Unit = runner.doTest That's all you need to run a parser or unparser test case named "testCaseName". DAFFODIL-2958
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.
+1, but the dependency should be removed form the val test
and added only to daffodil-tdml.
Personally, I don't really like this approach but I don't block it. Feels like it's a bit of a hack and that we should really use something that actually gets the name of a test. JUnit has that ability but it's much more verbose: https://github.com/junit-team/junit4/wiki/Rules#testname-rule. I wonder if a different test framework provides it. We barely use JUNit for most of our tests so something else might be a welcome change if it makes this kind of thing easier.
DAFFODIL-2958
Not sure if this is much better, but here's an alterantive approach that just uses JUnit: /** Defined in the a test library **/
trait EasyTestSuite {
implicit protected val _name: TestName = new TestName()
@Rule def name = _name
def doTest(runner: Runner)(implicit name: TestName): Unit = {
runner.runOneTest(name.getMethodName)
}
}
/** the actual test suite **/
class TestSuite extends EasyTestSuite {
@Test def test1(): Unit = doTest(runner)
@Test def test2(): Unit = doTest(runner)
...
} Accomplishes something similar, though not exactly the same. The biggest drawbacks are:
Maybe another consideration with either approach, is it can't be used with older versions of |
DAFFODIL-2958
This lets you use the name of the test method implicitly as the name of the TDML test it will run.
For example
That's all you need to run a parser or unparser test case named "testCaseName".
DAFFODIL-2958