Skip to content

Conversation

@zzambers
Copy link
Collaborator

This adds support for testing of KDF API. (see JDK-8331008.
Fixes: #72

Currently reflection has to be, so that tests support all jdks... :(
(I have included comment with original code, without reflection)

@judovana
Copy link
Collaborator

Despite well done and readable, the number of reflection is pretty big here. Similar approaches have already multiplied few times, and I think we should incorporate logic to go jdk by jdk. This project do not have binary releases, so you clone, build and try/test by same jdk. For jtreg-like approach, the jdk.version.major would be enough (although the one big test running all will need sme tuning). The make file can have detection too.

Qurestion is whether to take this PR as oportunity to move in this direction, or merge, and work on removal of reflection as another step. WDYT?

@judovana
Copy link
Collaborator

 grep -ir reflect -l | grep java
cryptotest/tests/KeyFactoryTests.java
cryptotest/tests/KEMTests.java
cryptotest/tests/CipherTests.java
cryptotest/tests/GssApiMechanismTests.java
cryptotest/utils/KeysNaiveGenerator.java
cryptotest/utils/Misc.java

Slilghtly less the I thought.

@zzambers
Copy link
Collaborator Author

zzambers commented Mar 12, 2025

@judovana there are 2 test classes done completely in reflection that it KEMTests and now KDFTests. (I think other ones are sporadic uses to retrieve some internal class etc...)

Unfortunately avoiding reflection is not so easy as it may seem here. AFAIK list test of sources cannot be conditional based on jdk, so we would need to duplicate top level @test with all it's dependencies for each jdk which introduced new APIs... Which is not much better. :( Also as I have discovered, there is another fun part and that is dealing with preview features (btw. KDFTests are preview in jdk24). That needs additional compiler and runtime switches (not needed, when reflection is used).

@zzambers
Copy link
Collaborator Author

And in makefile I would need to deal with these problems too... :(

@judovana
Copy link
Collaborator

And in makefile I would need to deal with these problems too... :(

I know that in makefile it would need much more care:( be sure I keep t in mind!-) However, manual listing of classes as is often much more easier then in other build tools. The other concerns you highlighted are already leading to some configure like is Kem supported.... yes is Kdf there... no and so on... ...brain rolling on....

@judovana
Copy link
Collaborator

The top level @test actualy is the only one which may be alowed to use reflection. But it is also the last one to be solved.

@judovana
Copy link
Collaborator

Maybe the jtregs conditionions (aka requires.extraPropDefns = VarDeps.java\n requires.properties = ...) may be used for jtreg parts, but that would complicate makefiles. However.. makefile an use the requires.extraPropDefns cass to get the same info...

@zzambers
Copy link
Collaborator Author

They would need to be excluded from compilation. We would still need add additional dummy files with different '@test' configurations for top level target... :( Unfortunately nor reflection in top level class nor requires.extraPropDefns would help with excluding tests from source list of top level target.

@judovana
Copy link
Collaborator

Ok, lets merge thsi now. But I will look deeper into the reflection removal.

@judovana judovana merged commit e7518c8 into rh-openjdk:master Mar 12, 2025
16 checks passed
@zzambers
Copy link
Collaborator Author

Btw, we will probably also have opposite problem, where some APIs will only work on older JDKs. As our tests use some APIs deprecated for removal. See: #79

@judovana
Copy link
Collaborator

Yup. Sure thing... #80 filled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cryptotest/CryptoTest.java fails by openjdk main-line repo

2 participants