On 2020-06-16 15:06, Erik Joelsson wrote:
On 2020-06-15 17:39, Igor Ignatyev wrote:
@David, Erik, Magnus,
please find the answers to your comments at the bottom of this email.
@all,
David's and Erik's comments made me realize that some parts of the
original patch were stashed away and didn't make it to webrev.00. I'm
truly sorry for the confusion and inconvenience. I also agree w/
David that it's hard to follow/review, and my gaffe just made it
worse, therefore I'd like to start it over again from a clean sate
http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02
Build changes look good to me now, except for a missing newline in
Main.gmk. (No need for new review.)
Ditto.
/Magnus
/Erik
833 lines changed: 228 ins; 591 del; 14 mod;
could you please review the patch which puts all tests for common
testlibrary classes (which is ones located in /test/lib) into one
location under /test/lib-test? please note this intentionally doesn't
move all "testlibrary" tests, e.g. tests for CTW, hotspot-specific
testlibrary, are left in /test/hotspot/jtreg/testlibrary_tests/ctw .
to simplify review, the patch has been split into several webrevs,
with each of them accomplishes a more-or-less distinct thing, but is
not necessary self-contained:
0. trivial move of tests from test/jdk and test/hotspot/jtreg test
suites to test/lib-test:
http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.00-move/
1. removal of hotspot's AssertsTest, OutputAnalyzerReportingTest and
"merge" of hotspot's and jdk's OutputAnalyzerTest:
http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.01-merge/
2. simplification of TestNativeProcessBuilder tests: converts Test
class used by TestNativeProcessBuilder into a static nested class:
http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.02-TestNativeProcessBuilder
3. makefiles changes to build native parts of lib-test tests. in
past, there were no tests w/ native parts in this test suite,
TestNativeProcessBuilder is the 1st such test; this part also removes
now unneeded lines from hotspot test suite makefile:
http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.03-NativeLibTest
4. clean ups in hotspot test suite: adjusted location
of SimpleClassFileLoadHookTest test, which is a test for
hotspot-specific test library (located in
/test/hotspot/jtreg/testlibrary/jvmti) and hence should not be moved
to /test/lib-test; removed TestMutuallyExclusivePlatformPredicates
from TEST.groups:
http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.04-hotspot/
5. LingeredAppTest fix (which apparently has never been run before):
http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.05-LingeredAppTest/
6. changes to make test/lib-test a fully supported and working test
suite, and add in into tier1, includes adding ProblemList,
TEST.groups file, and 'randomness' k/w:
http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.06-TESTROOT/
webrev: http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02
JBS: https://bugs.openjdk.java.net/browse/JDK-8211977
testing:
- make test TEST=tier1 locally on macosx
- test/lib-test on {windows,linux,macosx}-x64
- tier1 job (in progress)
Thanks,
-- Igor
On Jun 14, 2020, at 11:23 PM, David Holmes <david.hol...@oracle.com
<mailto:david.hol...@oracle.com>> wrote:
<...>
This doesn't seem to move everything under
test/hotspot/jtreg/testlibrary_tests/ e.g. ctw directory.
this is intentionally, ctw test-library is hotspot specific, hence
its tests are to reside in hotspot test suite (until we decide to
move the library to /test/lib), the same is true
for SimpleClassFileLoadHookTest.
You did not modify hotspot/jtreg/TEST.groups but it refers to:
testlibrary_tests/TestMutuallyExclusivePlatformPredicates.java \
Plus it should probably refer to the new native_sanity tests somewhere.
the actual version of the patch removed reference to
TestMutuallyExclusivePlatformPredicates from hotspot's TEST.groups
and had TestNativeProcessBuilder moved to /test/test-lib, so no
updates w.r.t. native_sanity are needed
The newly added copyright notice needs to have the correct initial
copyright year based on when the file was first added to the repo.
/test/lib-test/TEST.ROOT file was created by JDK-8243010 on
2020-04-16, hence the added copyright has 2020 as the initial
copyright year.
You used the wrong license header - makefiles don't use the
classpath exception.
JtregNativeLibTest.gmk is based on make/test/JtregNativeJdk.gmk which
also have classpath exception. quick grep showed that make directory
has 794 files which has '"Classpath" exception', out of 850 which
contain 'GNU General Public License version 2' string. And although I
agree that makefiles shouldn't have the classpath exception, I'd
prefer to keep JtregNativeLibTest.gmk in sync w/ the its siblings and
would leave it up to build team to decide how it's best to handle.
On Jun 15, 2020, at 8:12 AM, Magnus Ihse Bursie
<magnus.ihse.bur...@oracle.com
<mailto:magnus.ihse.bur...@oracle.com>> wrote:
A few comments:
This seems like code copied from elsewhere:
57 # This evaluation is expensive and should only be done if this target was
58 # explicitly called.
59 ifneq ($(filter build-test-libtest-jtreg-native, $(MAKECMDGOALS)), )
I don't agree that this is an expensive evaluation. Furthermore, the
makefile is only called for building the testlib and for making
images, so in worst case it's just the image part that would get a
penalty (although I highly doubt there is any).
right, JtregNativeLibTest.gmk is based
on make/test/JtregNativeJdk.gmk which has this comment, I don't know
enough details to say if it's expensive or not, yet if you insist and
there is a consensus within build team, I can remove the comment.
82 $(eval $(call SetupCopyFiles,COPY_LIBTEST_JTREG_NATIVE, \
Please use space after comma.
this again was preexisting in JtregNativeJdk.gmk, added the space
nevertheless.
On Jun 15, 2020, at 6:16 AM, Erik Joelsson <erik.joels...@oracle.com
<mailto:erik.joels...@oracle.com>> wrote:
In JtretNativeLibTest.gmk, lines 51-55 should probably be removed
(or adjusted if linking to libjvm is actually needed).
the lines were updated to define
BUILD_LIBTEST_JTREG_EXECUTABLES_LIBS_exejvm-test-launcher