Skip to content
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

Upgrade com.sun.tdk.signaturetest.SignatureTest in sigtest-maven-plugin #29

Open
alwin-joseph opened this issue Feb 22, 2024 · 5 comments
Labels
enhancement New feature or request

Comments

@alwin-joseph
Copy link
Contributor

alwin-joseph commented Feb 22, 2024

Upgrade com.sun.tdk.signaturetest.SignatureTest in sigtest-maven-plugin to be executed as a maven goal.

Context:
The signature tests in platform-tck and several standalone TCKs that were separated out, uses a legacy signaturetest framework majorly to invoke the sigtest-maven-plugin with specific arguments.
For eg: The runSignatureTest method is used to invoke the com.sun.tdk.signaturetest.SignatureTest with the right arguments to run the Signature Tests.

The org/netbeans/apitest/SigtestCheck.java can be run using maven goal "check" specified like here for CDI TCK.

Expected:
Can we also improve the com.sun.tdk.signaturetest.SignatureTest so as to execute the same via maven goal and parameters specified. This can avoid the complexities of the legacy signaturetest module in platform-tck and dependency on it.

cc @scottmarlow @gurunrao @arjantijms @jamezp

@alwin-joseph alwin-joseph added the enhancement New feature or request label Feb 22, 2024
@scottmarlow
Copy link
Member

Is this needed for the Expression Language 6.0 TCK?

Note that SigtestHandler is run from https://github.com/eclipse-ee4j/jakartaee-tck-tools/blob/master/tools/sigtest/src/main/java/org/netbeans/apitest/SigtestCheck.java.

The ignored jdk classes list should be checked in src/main/java/org/netbeans/apitest/SigtestHandler.java which should cover the equivalent of -IgnoreJDKClass

There is some support already for capturing the log output via org.netbeans.apitest.SigtestHandler and below code but could use more changes.

For reference the general documentation is here https://github.com/eclipse-ee4j/jakartaee-tck-tools/tree/master/tools/sigtest.

@jamezp
Copy link
Contributor

jamezp commented Feb 26, 2024

TBH I think we need to think about how these are ran in general. With the maven plugin the class path is setup with Maven dependencies from the class path. IMO that's wrong as it should only be using dependencies defined in an explicit class path. In other words, dependencies from a server not Maven. We need to verify the signature tests are ran against the libraries the server is shipped with.

That said, when I was doing some testing in the jakarta/rest TCK, the signature tests always passed for me even though they failed with the old manual test method. I didn't dig too much into it, but before we do that we should definitely look into why the check would pass.

@scottmarlow
Copy link
Member

scottmarlow commented Feb 26, 2024

I agree with @jamezp that the Jakarta EE Platform TCK signature tests need to deploy to all of the same EE containers that we test with currently in earlier EE releases.

So for Common Annotations I think that only needs a Java SE environment (e.g. no Platform or Web Profile implementation).

The same for the Standalone Expression Language TCK but the Expression Language tests (e.g. not the signature test) will definitely need to deploy to Web containers for the Platform/Web Profile TCK test runs.

@alwin-joseph
Copy link
Contributor Author

alwin-joseph commented Feb 27, 2024

Is this needed for the Expression Language 6.0 TCK?

This enhancement is not a mandatory requirement. This was motivated from the below comment.

Also, in Faces and REST there is an enormous amount of extra code for checking the signature. Is that really needed?
Why doesn’t the maven plugin do the check?

The signaturetest module in tckrefactor is currently used by all standalone and platform TCKs to invoke the com.sun.tdk.signaturetest.SignatureTest in the plugin, in static and reflective modes.
Whereas for CDI TCK the org/netbeans/apitest/SigtestCheck.java can be run using maven goal "check" specified like here. IIUC the same org/netbeans/apitest/SigtestCheck cannot be used by other TCKs as I do see differences with com.sun.tdk.signaturetest.SignatureTest and org/netbeans/apitest/SigtestCheck especially the static and reflective modes in com.sun.tdk.signaturetest.SignatureTest.

So IMO if we improve the com.sun.tdk.signaturetest.SignatureTest to be invoked how the signaturetest module does currently similar to the goal "check" in org/netbeans/apitest/SigtestCheck , we could remove the large amount of code in signaturetest module to make it easier for vendors.

PS: For EL TCK , the PR jakartaee/platform-tck#1227 was made to copy the only necessary signaturetest module sources into the EL TCK so as to remove the dependency of EL TCK on signaturetest module making it independent. This is not related to this issue but from the requirement that signaturetest module(and related dependency modules like libutil, common, runtime ) would need to be published in maven if we are running EL TCK separately.

@scottmarlow
Copy link
Member

Is this needed for the Expression Language 6.0 TCK?

This enhancement is not a mandatory requirement. This was motivated from the below comment.

From https://wildfly.zulipchat.com/#narrow/stream/242838-Jakarta-EE/topic/Signature.20TCK.20testing thread, James mentioned the problems with the maven sigtest plugin.

For static testing (e.g. Java SE style classpath specified) we talked about adding a Maven Plugin property for passing the (static) classpath property in (we need to a unique name for that property that is not likely to be used for other things, classpath seems too generic).

For reflective testing (e.g. deploying signature tests to an EE container, I don't think the Maven Plugin will control the deployment.

Also, in Faces and REST there is an enormous amount of extra code for checking the signature. Is that really needed? Why doesn’t the maven plugin do the check?

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants