Test TCK compliance

[keilw]

There is a branch https://github.com/unitsofmeasurement/unit-tck-usage/tree/seshat for testing Seshat against the TCK.

[keilw]

@desruisseaux Did SIS pass the 1.0.1 TCK earlier? Please feel free to adopt the TCK usage demo where necessary. I found it tricky because all implementing classes for Unit etc. seem hidden behind package-private visibility, so hard to say, how the TCK may be configured to find those for basic tests e.g. in section 4.1?

[desruisseaux]

I have run the TCK on Apache SIS a while ago. Indeed, implementation classes are package-privated (not designed for being extended). If the issue is about providing a tec.units.tck.util.ServiceConfiguration, the usual approach is to provide such implementation in the same package than the package-private classes to be tested.

[desruisseaux]

Ported TestConfiguration and META-INF/services/tec.units.tck.util.ServiceConfiguration from unit-tck-usage seshat branch to the main Seshat project and tried to add unit-tck as a Maven test dependency, but it cause the following error message at build time:

[TestNG] [ERROR] No test suite found. Nothing to run
Usage: <main class> [options] The XML suite files to run
  Options:
    -configfailurepolicy
       Configuration failure policy (skip or continue)
    ...etc...

How are we suppose to use the TCK in a Maven build?

[keilw]

It's all set up in the tck-usage repository.

Could you make the necessary adjustments, e.g. matching package in the https://github.com/unitsofmeasurement/unit-tck-usage/tree/seshat branch. As soon as something is pushed to any branch, the build starts. This is the TestNG report for the EDR stage of Indriya: https://104-41443578-gh.circle-artifacts.com/0/home/circleci/repo/target/tck-output/index.html Implementations are free to test it locally, but this way full transparence of a compatibility claim is possible, something e.g. Jakarta EE hopes to find ways how this could also be done in a similar way at Eclipse.org.

Well, as long as there is not even a Snapshot build of Seshat, it probably won't work there, but it should work locally. Can we see the ServiceConfiguration you updated somewhere?

[keilw]

If it worked better to deploy Snapshot builds of Seshat into the Geotoolkit.org repository that would also be an option.

[keilw]

After #6 was done and snapshots get deployed, the seshat branch of unit-tck-usage runs through. However, there are still 2 failures: https://117-41443578-gh.circle-artifacts.com/0/home/circleci/repo/target/tck-output/tck-results.txt Seshat does not contain a concrete Quantity implementation, so even if you pass just Quantity as such to the test configuration, the interface is missing hashCode() and equals().

[desruisseaux]

I added a test case for Quantity.equals(Object) and hashCode() implementations and verified that those methods work as intended. The TCK test which is failing checks for explicit equals and hashCode methods in implementation. But Seshat implementation is, in some case, a java.lang.reflect.Proxy class which does not declare explicitly those methods. But it inherits them from java.lang.Object and I verified that they are invoked as expected.

Proposed fixes:

We can declare the equals and hashCode methods in the Quantity interface. This is a compatible change since an implementation of those methods is inherited from java.lang.Object, so no code will be broken. I think it is desirable because if the TCK is testing those methods, it means that we expect some contract to be met. We are better to document this contract explicitly in Javadoc.

The TCK TestUtils.testHasPublicMethod method currently gets the list of methods by a call to Class.getDeclaredMethods(), filter the public methods, then search in super-classes (source code). I think we should just invoke Class.getMethods() instead, which will perform all those steps for us (no need anymore for the loop). I'm not sure why the current code does not see the equals and hashCode methods in Proxy case, but my hope is that the use of getMethods() would work around this issue.

[keilw]

I guess we could do that, like the toString() method in some cases but it would require an API change. Whether that's a trival tiny change we're able to declare "2.0.1" and release on the fly or it needs a MR, I'll ask @otaviojava and the PMO as they are all at OC1 right now. It seems we did win a JCP Award for the JSR last night. Stay tuned for that on the orga page later this week.

[desruisseaux]

The proposed change in Quantity interface is low priority and can be delayed until there is more material for an eventual later version. For the proposed changes in the TCK, we would need to test if it solve the issue.

[keilw]

Is there a PR for the TCK? And if so, which branches of the TCK does it make sense for? If Seshat intends to implement JSR 385 in the near future, then maybe stick to master/2.0, if it stays with 1.0 for some time, then we could also do something there. Please if there is a PR file it/them in the appropriate branch(es)

[desruisseaux]

Patch is not yet created. My problem is that I do not know how to test it? Do you have a link to a documentation about how to execute the TCK from command-line?

[keilw]

It is both in https://github.com/unitsofmeasurement/unit-tck, but you may want to run it via https://github.com/unitsofmeasurement/unit-tck-usage. There is a RunTCK class, but you may want to use Maven like in the https://github.com/unitsofmeasurement/unit-tck-usage/tree/seshat branch. Because Seshat has many package-local classes, the TestConfiguration had to be refactored to its own package, but that seems perfectly fine. If e.g. the README in the TCK Usage project was too short or you find something could be added, please provide a PR there as well if you can.

[keilw]

I also try to look at the Seshat GH-page branch later today.

[desruisseaux]

TCK tests have been integrated to the build and are executed at every mvn clean install invocation. One issue is that the clean phase is currently mandatory. To avoid this limitation, I did not yet found anyway other than bypassing ServiceLoader for the particular case of ServiceConfiguration (it is still fine to use ServiceLoader for ServiceProvider).

[keilw]

What is the problem with the clean phase? And is this specific fo Seshat being tested or specific to the TCK? I am not aware it would be mandatory but it is a good thing go run because not everywhere the Docker container or similar VM is recreated every time.

[desruisseaux]

The build failure is generic to any implementation compiled as Jigsaw module, unless I missed some Jigsaw configuration that would allow to avoid the problem.

The relationship with the clean phase is specific to Seashat. It is because Seashat creates a multi-versions JAR file by executing build steps in this order:

1. Compile for Java 8.
2. Test.
3. Compile for Java 9 as Jigsaw module.

So the Jigsaw modularisation happens after the tests. But if the clean phase has not been executed, then the Java 9 classes are present at test time as if the tests happened after step 3. It causes the problem described in next paragraph.

The problem with Jigsaw is that I have not found any way to declare a service (in this case ServiceConfiguration) at test time only, without declaring that service in the main code. Putting a file in src/test/resources/META-INF/services works for non-Jigsaw module but not with Jigsaw. But maybe some way exists and I just didn't found it.

This problem does not happen with tests in tck-usage repository for instances because those tests are in a Maven project separated from the library to be tested. The problem described in above paragraph happens when we want to execute the TCK in the main project instead than in a separated project.