On Tue, 30 Apr 2024 12:08:21 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> Can I please get a review of these test-only changes which proposes to
>> remove the `test/jdk/sun/tools/jcmd/JcmdOutputEncodingTest.java` and
>> `test/jdk/sun/tools/jstack/BasicJStackTest.java` tests from
>> `ProblemList-Virtual.txt`?
>>
>> When jtreg was enhanced to allow running the tests from within a virtual
>> (main) thread, some tests were problem listed since they either were failing
>> at that time or the test code would require additional work to make it work
>> when the current thread is a virtual thread. The
>> `test/jdk/sun/tools/jcmd/JcmdOutputEncodingTest.java` and
>> `test/jdk/sun/tools/jstack/BasicJStackTest.java` are 2 such threads which
>> require special handling when the current thread is a virtual thread.
>>
>> `test/jdk/sun/tools/jcmd/JcmdOutputEncodingTest.java` has been updated to
>> use a different command to dump stacktraces when the test runs in a virtual
>> thread. I went back and looked at the original issue for which this test was
>> introduced and based on that, using a different command to dump the
>> stacktraces shouldn't impact what the test was originally intended to test.
>>
>> `test/jdk/sun/tools/jstack/BasicJStackTest.java` has been updated to be
>> skipped when the current thread is the virtual thread. This is because the
>> test exercises the `jstack` tool which doesn't print stacktraces of virtual
>> threads and thus the test isn't applicable for virtual threads.
>>
>> I've run these tests with this change, both with platform threads and
>> virtual threads in our CI and the tests complete without failures.
>
> Jaikiran Pai has updated the pull request incrementally with one additional
> commit since the last revision:
>
> address additional problem listed tests in hotspot/jtreg/serviceability
test/hotspot/jtreg/serviceability/tmtools/jstack/DaemonThreadTest.java line 77:
> 75: t.setDaemon(false);
> 76: }
> 77: testThread(t, "");
I think the following is a bit clearer:
// Virtual threads are always daemon threads. Therefore if the current thread
is a
// virtual thread, then NormalThread will be a daemon thread and we need to
explicitly
// make it not a daemon thread.
You don't actually need the isVirtual() check, especially with the comment
explaining things. However, you could also forgo all this logic and the comment
by just sticking setDaemon(false) in the NormalThread constructor.
test/jdk/sun/tools/jstack/BasicJStackTest.java line 53:
> 51: // print the stacktraces of virtual threads.
> 52: throw new SkippedException("skipping test since current
> thread is a virtual thread");
> 53: }
I wonder if we can rely on the virtual thread always be mounted. If so, we
could revisit this test after
[JDK-8330846](https://bugs.openjdk.org/browse/JDK-8330846) is implemented.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/19016#discussion_r1586668634
PR Review Comment: https://git.openjdk.org/jdk/pull/19016#discussion_r1586695143
