Hi Igor,
On 16/06/2020 10:39 am, 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
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 .
Ok.
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:
Many thanks for doing this!
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/
Looks good.
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/
Looks good.
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
Looks good.
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
Hmm okay. Not sure this needed its own category of build logic but ...
Aside: Makefiles should not have the classpath exception version of the
license header. But they all do for some reason. I'll check if we need
to do a separate cleanup of that.
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/
Looks good. Took me a while to understand the connection to the test
library here.
5. LingeredAppTest fix (which apparently has never been run before):
http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.05-LingeredAppTest/
Someone from serviceability should evaluate this test. It may not be needed.
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/
Seems okay.
Thanks,
David
-----
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