unitsofmeasurement / unit-tck

JSR 385 - Technology Compatibility Kit (TCK)
Other
6 stars 5 forks source link

Allow developers to specify the configuration programmatically. #43

Closed desruisseaux closed 1 year ago

desruisseaux commented 2 years ago

The intent is to allow execution of tests in the same Maven project than the implementation, instead of creating a separated Maven project. We can not always use the ServiceProvider mechanism because we may not want to add a provides clause in module-info.java of the main code.

This is the same goal than #32 but coded differently. Despite being labelled as "merged", #32 does not appears in the code.


This change is Reviewable

desruisseaux commented 2 years ago

Done. I also modified the proposal with an initialize(…) method instead of setConfiguration(…). The intent is to make sure that the configuration can not be changed after the tests started.

keilw commented 2 years ago

How does this look for testers/implementers if they want to customize the TCK run? There are two places:

If required we may also have to revisit or update https://github.com/unitsofmeasurement/unit-tck-usage

desruisseaux commented 2 years ago

I think it does not change anything to those documentation. In particular, this commit does not add any new parameter to supply on the command line. It rather provides an alternative way to run the TCK. Instead of the following steps documented in README.md:

  1. Create a new Maven project.
  2. Add this TCK and your implementation as dependency.
  3. Implement a class of type tech.units.tck.TestSetup,

We would have (note that there is no "create new Maven project step"):

  1. Add this TCK as a dependency in your project with <scope>test</scope>.
  2. Add a test class implementing tech.units.tck.util.ServiceConfiguration.
  3. Add a test method like below:
import tech.units.tck.TCKSetup;
import tech.units.tck.TCKRunner;

public class MyTest implements ServiceConfiguration
    @Test
    public void runTCK() {
        TCKSetup.initialize(this);
        final TCKRunner runner = new TCKRunner();
        int returnCode = runner.run(null, null, null);
        assertEquals("Some TCK tests failed.", 0, returnCode);        
    }
}

With that, the TCK is part of project test suite and is run every time that the project is build. It does not require that we create and run a separated project.

So it is not a change in current instruction. It would be a separated section for another way to run the TCK.

keilw commented 2 years ago

There is no "Create new Maven project" step, but we would have to somehow document it if not here then in the "blueprint" project https://github.com/unitsofmeasurement/unit-tck-usage.

It can be called very similar (TCKRunner implements the javax.tools.Tool interface)

public class RunTCKTest {

   @Test
   public void runTCK(){
      final Tool runner = new TCKRunner();
      int returnCode = runner.run(System.in, System.out, System.err, new String[0]);
      assertEquals(0, returnCode);
   }
}

I'm not sure, if above call of TCKSetup.initialize(this) really adds value compared to using the service loader in META-INF/services/tech.units.tck.util.ServiceConfiguration? Or (we should demonstrate this in a future version of the tck-usage demo) by declaring it in the module-info using JPMS, once the TCK itself also has one, see #47.

Except having MyTest1 and MyTest2 in a single project, but you might as well declare project1 and project2, the initialize() method does not allow e.g. changing anything at runtime, does it? That would IMO make more sense, for a build or compile time configuration the normal service loader mechanisms should be sufficient.

desruisseaux commented 1 year ago

Did the following changes in the README.md file:

However I do not know how to edit the https://unitsofmeasurement.github.io/unit-tck/ page.

keilw commented 1 year ago

You can make changes in the docs folder of this repo, mostly index.html, the rest should rarely change, unless you add further images.

desruisseaux commented 1 year ago

I propose to withdrawn this merge request. A few months ago I could get it working with Seshat, but it is no longer the case. TestNG and JUnit have numerous conflicts in their dependencies, so I abandon the attempt to integrate TCK tests in Seshat test suite. Instead I created a separated Maven project for running the TCK, which is what the current instructions propose to do.

Note that I still can't get the TCK to run, but this is a separated issue than this pull request.

keilw commented 1 year ago

Ok, then is the whole initialize() approach something we should no longer consider for MR2? Note I did have to change some test methods in the TCK after upgrading to the latest version of Reflections.io but it works now also in https://github.com/unitsofmeasurement/unit-tck-usage/. About the Maven project, what's wrong with https://github.com/unitsofmeasurement/unit-tck-usage/tree/seshat ? I know it is old, probably still at Seshat 1.0 but wouldn't it be possible to upgrade dependencies in that branch and run it there?

desruisseaux commented 1 year ago

My motivation for initialize(…) was to integrate the TCK with the Seshat test suite. I do not know if other use cases exist. Hearing none, we may conservatively abandon the whole initialize(…) approach.

The issues that I had with initialize(…) were conflicts in the dependencies. It is not necessarily TCK fault; some conflicts were between JUnit and TestNG. This experiment makes we realize that it is risky to mix two testing frameworks in the same environment. In theory they are supposed to coexist. In practice it is no trivial.

Regarding TCK execution using the current approach (separated Maven project), the issues that I had were related to the modularization of Seashat, which was done with the addition of an unconditional module-info file (not as an optional META-INF/releases/9 file). In a modularized project, Java become stricter on dependencies convergence. It is part of Jigsaw effort to resolve the "class path hell". Java reported many dependency conflicts that were not reported in a non-modularized project. I did not investigated yet the exact causes. So again it is not necessarily TCK fault. But for making sure that the TCK is okay in this aspect, it would be worth to add the following snippet in the <plugins> section of the TCK pom.xml file:

<plugin>
  <groupId>org.apache.maven.plugins</groupId>
  <artifactId>maven-enforcer-plugin</artifactId>
  <executions>
    <execution>
      <id>enforce</id>
      <configuration>
        <rules>
          <dependencyConvergence/>
        </rules>
      </configuration>
      <goals>
        <goal>enforce</goal>
      </goals>
    </execution>
  </executions>
</plugin>

In addition, if TCK Java version policy allows, it would be nice to upgrade minimum Java requirement to Java 9 (or 11) and add a module-info.java file to the TCK. Note that having the TCK in Java 11 does not mean that the Unit API needs to upgrade to Java 11 as well. Developers can write a Java 8 compatible implementations and test it with Java 11. But having the TCK as a Jigsaw JAR file would help to ensure that dependency convergence is okay.

keilw commented 1 year ago

Mixing JUnit and TestNG is not really recommended, we use JUnit 5 everywhere for module tests in the RI (Indriya) while TestNG is used for the TCK, and that likely won't change anytime soon (the main reason for the TCK was easier configuration even at runtime by passing XML definitions, JUnit may have equivalents but v4 did not have much then)

Regarding a module-info, there is a ticket #47 for that already. As no Multi-Release JAR is required, a compile-cycle similar to JSR 354 or Jakarta JSON Processing (2.0.x) instead of the Toolchain or AntRun plugins used here in most repositories.

We could in theory raise the minimum JDK version to 11 or 17 for the TCK, but given the API (until we come to a point where e.g. Vector API or something else, e.g. truly useful value objects) has the minimum JDK version of 8, it also feels better to keep the TCK aligned with that.