stevedlawrence commented on code in PR #1389: URL: https://github.com/apache/daffodil/pull/1389#discussion_r1880410848
########## daffodil-tdml-junit/src/main/scala/org/apache/daffodil/junit/tdml/TdmlSuite.scala: ########## @@ -0,0 +1,122 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.daffodil.junit.tdml + +import java.io.File + +import org.apache.daffodil.tdml.Runner +import org.apache.daffodil.tdml.TDMLTestNotCompatibleException + +import org.junit.AfterClass +import org.junit.AssumptionViolatedException +import org.junit.Rule +import org.junit.rules.TestName + +/** + * Mixin for a JUnit test suite companion object + */ +trait TdmlSuite { + + /** + * Resource path to the TDML file to use for this JUnit suite + */ + val tdmlResource: String + + /** + * Function to get the directory containing tdmlResource. Useful if createRunner is overridden + * to use the Runner(dir, file, ...) factory method + */ + def tdmlDir: String = new File(tdmlResource).getParent() + + /** + * Function to get basename of tdmlResource. Useful if createRunner is overridden to use the + * Runner(dir, file, ...) factory method + */ + def tdmlFile: String = new File(tdmlResource).getName() + + /** + * Default implementation to create a Runner using the tdmlResource. This function can be + * overridden if additional options need to be passed to the Runner factory object, for + * example to disable TDML validation. In most cases this should not be needed, since most + * parameters should not be changed by normal schema tests, or can be defined in the TDML + * file. + */ + def createRunner(): Runner = Runner(tdmlResource) + + /** + * Lazily build the runner when needed + */ + final lazy val runner = createRunner() + + /** + * Ensure all resources associated with the Runner (e.g. cached compiled schemas, parsed TDML + * XML) to be freed once the test suite has completed + */ + @AfterClass + final def shutDown(): Unit = runner.reset +} + +/** + * Mixin for a JUnit test suite companion class + */ +trait TdmlTests { Review Comment: While I am making modifications to all our test files, I wonder if it's worth considering how we identify tests as broken? Currently we just comment them out, e.g. ```scala //@Test def foo = test ``` One alternative to this might be to use the `@Ignore` annotation, and optionally provide a bug number in the description, e.g. ```scala @Ignore("DAFFODIL-1234") @Test def foo = test ``` Benefits: 1. It ensures the "foo" function is valid and isn't duplicated or something. 2. When running `sbt test`, it adds a line for each ignored test and at the end of testing it shows the total count of ignored tests. So there's a metric of how many tests are failing. However, this does make the test output a bit more verbose and if you're not aware of why a test might be ignored it might be confusing to see a bunch of ignored tests. 3. There is a clear place for bug number information, a convention that we could enforce Here's what it ignored tests look like in SBT:  Another approach is to add a new "knownToFail" method, which could also take an optional bug number, e.g. ```scala @Test def foo = knownToFail("DAFFODIL-123") ``` This has similar benefits to `@Ignore`, but the `knownToFail` method could make sure the test actually exists in a TDML file, so there aren't concerns of accidentally moving/deleting a failing test. A downside of this approach is the new function would need to cause an assumption failure to prevent it from actually causing a failure, and in SBT these assumption failures look kindof fatal even though they really aren't. So this might be confusing to new users to see a bunch of red "assumption failed" messages when running tests. We could make it so the error message would include something like "known to fail" but it still could be a bit intimidating. For reference, here's what this looks like in SBT:  Note that `@Ignore`'d tests show up in stats as "ignored", but "assumption failed" show up as skipped. Another option, similar to the above, is to add a new attribute to TDML test cases, e.g. ```xml <tdml:parserTestCase ... knownToFail="DAFFODIL-123"> ``` If a test were run with this attribute set it would throw an exception that would be turned into an assumption failure. So it gives the same result as the "knownToFail" method approach, but the Scala files for a failed test are exactly the same as for passing tests. This makes it really easy to generate test scala files from a TDML file since there are no differences between working or known-to-fail test. But otherwise this is the same as the knownToFail method, including the same drawbacks about verbosity. It's just now controlled by the TDML file and the TDML Runner instead of the scala file. Or, we just keep it as is. We've gone this far by just commenting out tests and that's been sufficient. Just thought this was a good time to consider alternatives since I'm already changing all these files and it potentially affects the TdmlTests API. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
