CI: Test on all current Java LTS versions (8, 11 ,17, and 21)

[allentiak]

Tests on JDK 17 and 21 are optional (since they currently fail).

The very existence of these tests will increase the awareness on the importance of code compatibility with newer Java versions, thus enabling future transitions to be implemented gradually.

Making the code run on JDK 11 took a lot of hacking. This will reduce the effort for eventual future transitions.

[tool4ever]

Looks like it's TestNG causing problems?
Would they even offer a version that can support 4 JDK simultaneously... 🤔
Otherwise I think seeing all PR fail (partly) is too noisy and confusing for contributors.

[allentiak]

Looks like it's TestNG causing problems? Would they even offer a version that can support 4 JDK simultaneously... 🤔 Otherwise I think seeing all PR fail (partly) is too noisy and confusing for contributors.

Well, it seems this PR is already achieving its aim of raising awareness on things to improve...
What about accepting this pain as an incentive to

1. deprecate TestNG?, and
2. move existing TestNG tests to JUnit 5?

Like this PR, for example: #4942

[tehdiplomat]

Don't you need to run your test with add-opens?

forge/forge-gui-desktop/src/main/config/forge.sh

Lines 44 to 52 in 152ad0b

	if [[ $v -ge 17 ]]
	then
	java --add-opens java.desktop/java.beans=ALL-UNNAMED --add-opens java.desktop/java.awt.color=ALL-UNNAMED --add-opens java.desktop/javax.swing.border=ALL-UNNAMED --add-opens java.desktop/javax.swing.event=ALL-UNNAMED --add-opens java.desktop/sun.awt.image=ALL-UNNAMED --add-opens java.desktop/sun.swing=ALL-UNNAMED --add-opens java.desktop/javax.swing=ALL-UNNAMED --add-opens java.desktop/java.awt=ALL-UNNAMED --add-opens java.base/java.util=ALL-UNNAMED --add-opens java.base/java.lang=ALL-UNNAMED --add-opens java.base/java.lang.reflect=ALL-UNNAMED --add-opens java.base/java.text=ALL-UNNAMED --add-opens java.desktop/java.awt.font=ALL-UNNAMED --add-opens java.desktop/java.awt.image=ALL-UNNAMED --add-opens java.base/jdk.internal.misc=ALL-UNNAMED --add-opens java.base/sun.nio.ch=ALL-UNNAMED --add-opens java.base/java.nio=ALL-UNNAMED --add-opens java.base/java.math=ALL-UNNAMED --add-opens java.base/java.util.concurrent=ALL-UNNAMED --add-opens java.base/java.net=ALL-UNNAMED -Dio.netty.tryReflectionSetAccessible=true $SHAREDPARAMS
	elif [[ $v -ge 11 ]]
	then
	java --illegal-access=permit $SHAREDPARAMS
	else
	java $SHAREDPARAMS
	fi

[allentiak]

Don't you need to run your test with add-opens?

Wouldn't this hide the fact that the current code does not compile with JDK 17+?

I think that it is important to be explicitly aware of this...
(aka "Bring the pain forward")
https://medium.com/continuousdelivery/if-it-hurts-do-it-more-often-f5a00cc12ffa

If we are not even aware of this problem, then no one will see an incentive to fix it...

As I said above, I think that just this awareness in itself makes merging this worth it...

[Hanmac]

Wouldn't this hide the fact that the current code does not compile with JDK 17+?

it would. if using the right ADD-OPENS parameters

[github-actions]

This PR has not been updated in a while nad has been marked on stale. Stale PRs will be auto closed

[allentiak]

Well, being explicit about that the code won't compile unless that hack is used is part of the "awareness raising" rationale I mentioned when I first opened this PR...

[allentiak]

I mean, the first step to solve a problem is to accept its existence, right?
https://jhall.io/archive/2022/06/11/if-it-hurts-do-it-more/

[Hanmac]

I mean, the first step to solve a problem is to accept its existence, right? https://jhall.io/archive/2022/06/11/if-it-hurts-do-it-more/

The "pain" you are referring too is JAVA 17+ messing up our Code we depend on it, and can't really update

[tehdiplomat]

Here's my suggestion for this PR. Instead of having these failing (optional) tests run every single PR which is a lot of noise and Github Actions CPU cycles. And potentially very noisy for new people who don't understand that this is expected behavior, why don't we associate this change only during release builds that way we can keep it in mind if there's a future way to avoid add-opens, but its not just noise for the sake of noise.