On Wed, 5 Apr 2023 20:09:38 GMT, Chris Plummer <cjplum...@openjdk.org> wrote:
>> I'd prefer to always use the "test.class.path" as it is a complete classpath >> required to run test. >> >>> The better comment would be >>> // When we run jtreg with plugin, the full classpath including testlibrary >>> is required to use TestScaffold is required >> >> I guess, you are going to remove this duplication with "is required". :) > > I think the CR should explain this test.classes vs test.class.path > difference, and also explain why the classpath requirements are different > when using virtual threads. You say that "TestScaffold should be also in > classpath", but don't explain why. It's also not clear to me why only tests > that are in subdirs are impacted when running with the wrapper. > > Also, when you say "When we run jtreg with plugin..", do you mean with the > wrapper, not the plugin? @plummercj, I'e updated CR with more detailed information. Hope it makes clearer why this change is needed. Here is the text. " VMConnection runs debuggee using "test.classes" as a classpath for debuggee classes. It works fine when test and TestScaffold.java are located in the same directory and are both compiled into "test.classes" location. However, it causes test failures when the virtual thread factory (wrapper) is enabled and the test is not located in the same directory as TestScaffold. Such tests use TestScaffold as a testlibrary. (They have ' * @library ..' or something like similar). So TestScaffold is not included in 'test.classes' which include tests only. However, it is included as all testlibrary classes in "test.class.path" classparth. It is needed to use "test.class.path" to be able to use TestScaffolld as debugee wrapper Might be it better to always use "test.class.path" as classpath for debugee to minimize the difference between standard and "virtual" execution. " Yes, it would be better to change "When we jtreg with plugin" to "When 'main wrapper' is used". Before updating the comments I want to confirm if we want to keep "test.classes" for standard execution or change it to use "test.class.path" always. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/13339#discussion_r1159047948
