Usage of Java assertions in JTreg tests?

[Volker Simonis]

I didn't intend to change tests to use `/othervm` just to enable assertions.

But for tests which already do use `/othervm` anyway and also use
assertions it would be trivial to add "-ea" to the command line.

I agree that we should audit the tests and decide individually which
tests should be changed to run with "-ea" and which Java assertions
should better changed to either testNG/JUnit assertions or manually
written assert functions.

Best regards,
Volker

On Thu, Nov 17, 2022 at 5:35 PM Jonathan Gibbons
<jonathan.gibbons at oracle.com> wrote:
>
>
> On 11/16/22 5:48 AM, Volker Simonis wrote:
> > On Tue, Nov 15, 2022 at 6:41 PM Alan Bateman <Alan.Bateman at oracle.com> wrote:
> >> On 15/11/2022 17:19, Volker Simonis wrote:
> >>> Hi,
> >>>
> >>> I recently stumbled upon
> >>> test/jdk/java/lang/invoke/JavaDocExamplesTest.java which uses a lot of
> >>> Java assertions like this:
> >>>
> >>> for (int n = 0; n <= 10; n++) {
> >>>     Object[] badArityArgs = (n == 2 ? new Object[0] : new Object[n]);
> >>>     try { assert((boolean) eq2.invokeExact(badArityArgs) && false); }
> >>>     catch (IllegalArgumentException ex) { } // OK
> >>> }
> >>>
> >>> The intent of this test is to verify that invokeExact() always throws
> >>> an exception because otherwise the assertion will fire. Unfortunately,
> >>> this test only does something at all if JVM is running with "-ea"
> >>> which is not the default. In the standard configuration, all the code
> >>> inside the "assert(..)" will just be skipped and the test always
> >>> passes.
> >>>
> >>> This example (and a lot more under test/jdk/java/lang/invoke/) is
> >>> there since JDK 7. The tests also use testNG assertions a lot (e.g.
> >>> "assertEqual()", "assertTrue()", etc.) so I wonder if the plain java
> >>> asserts shown in the example above are just typos and should be
> >>> replaced by application level testNG assertions? What are these tests
> >>> good for otherwise if they won't be executed most of the time?
> >>>
> >> I don't know the history of this test to know if it really meant to rely
> >> on running with -ea or not.  Given that it has assertXXX methods then it
> >> may have been partly converted to TestNG at some point. However, I don't
> >> think they are as useless as it might seem. RunTests.gmk (the make file
> >> that is used when use you `make run-test ...`) adds -ea -esa to the
> >> command line when running non-hotspot tests so the assertions are
> >> enabled, which is useful when running tests in areas that do make use of
> >> assert.
> > Thanks for the info. I usually don't run the tests through the make target.
> >
> > I think testing would be much more reliable if all the tests which
> > depend on Java assertion would explicitly run with assertions  enabled
> > (i.e. "@run main/othervm -ea"). Would you agree to such a change?
>
> We should not change tests to use `/othervm` just to enable assertions.
> That would have a negative impact on performance, if the tests are
> normally run in agent mode.
>
> It may be worth auditing the tests to see how many tests use `assert`
> and which may be run without assertions enabled. Depending on that
> info, we can decide a better course of action.
>
> -- Jon
>
>
> >
> > A lot of tests already do this (e.g. test/jdk/jdk/incubator/vector/)
> > but I also found completely useless hotspot tests which rely on
> > assertions but by default run without "-ea" even if started from the
> > "make test" target (e.g.
> > test/hotspot/jtreg/compiler/c2/TestSignExtract.java). They should
> > definitely be fixed. If you don't mind, I'll create JBS issues for
> > these tasks.
> >
> > Best regards,
> > Volker
> >
> >> -Alan