Failing terminal tests in I-Build

[HeikoKlare]

Several (175) terminal tests are failing in I-Build test execution because they are expected to be run with assertions enabled, but the Ant-based execution does not seem to do that by default:

No Assertions! Run this code with assertions enabled! (vmargs: -ea)

java.lang.Error: No Assertions! Run this code with assertions enabled! (vmargs: -ea)
at org.eclipse.terminal.internal.model.AbstractITerminalTextDataTest.setUp(AbstractITerminalTextDataTest.java:34)
at java.base/java.lang.reflect.Method.invoke(Method.java:580)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)

It affects every test class derived from AbstractITerminalTextDataTest.

See, e.g., https://download.eclipse.org/eclipse/downloads/drops4/I20260106-1800/testresults/html/org.eclipse.terminal.test_ep439I-unit-win32-x86_64-java21_win32.win32.x86_64_21.html

[akurtakov]

Shouldn't this check be removed? AFAIK JUnit doesn't have such requirements.

[HeikoKlare]

Probably, yes. I wanted to take a look whether there is something in the production code or tests that requires JVM assertions to enabled, but we should see when just removing that check.

[akurtakov]

A quick look points to cases like

eclipse.platform/terminal/bundles/org.eclipse.terminal.control/src/org/eclipse/terminal/internal/model/TerminalTextDataStore.java

Line 65 in 53226b1

assert height >= 0 || throwRuntimeException();

and many other such cases .

IMO this is extremely bad pattern as if the JVM is not started with -ea there will be no RuntimeException with all possible problems in calculations coming from that. Even throwing RuntimeException with no further details is a bad idea, what should be done is throwing IllegalArgumentException with a clear message what is wrong and adjust tests.

[HeikoKlare]

IMO this is extremely bad pattern as if the JVM is not started with -ea there will be no RuntimeException with all possible problems in calculations coming from that. Even throwing RuntimeException with no further details is a bad idea, what should be done is throwing IllegalArgumentException with a clear message what is wrong and adjust tests.

I agree. I guess the idea was to use such assertions to evaluate pre- and postconditions of methods during tests but to avoid exceptions at productive runtime (or rather expect that they do not occur at productive runtime, because you have found them when testing). They are different from JUnit assertion as you do not assert specific selective behavior but assert that contracts are fulfilled across usage scenarios. I remember that this used to be a not-so-uncommon pattern around 15 years ago, but still think it is actually bad practice.
In case those contracts are supposed to be evaluated, I agree that this should always be done and not just when having VM assertions enabled. So I agree with your proposal of throwing appropriate exception types (like IllegalArgumentException or whatever is appropriate in the individual case) without guarding them with an assertion.

[HeikoKlare]

Today we have only 25 tests fails after removing the check for the -ea VM argument.
Those remaining test failures seem to be caused by several code places not throwing an exception due to assertions being disabled.

[HeikoKlare]

No failing terminal tests in yesterday's I-Build anymore: https://download.eclipse.org/eclipse/downloads/drops4/I20260108-1800/tests.html

Thank you, @akurtakov!