[10] RFR of JDK-8173411: Some testng tests check nothing in java time
Please review the patch to change TestNG.dirs and lib.dirs (def and file structure) for test/jdk/java/time. test/jdk/java/time contains three sets of tests: nontestng/ tck/ test/ Tests from directory "tck" and "test" are testng tests with properties: TestNG.dirs = .. lib.dirs = ../../../lib/testlibrary lib.build = jdk.testlibrary.RandomFactory But not all files under "TestNG.dirs" are real tests, some of them are "libraries" thus should not be put under "TestNG.dirs" (thus be run as testng test). Moreover, due to this def, when one runs tests from "test" directory, extra files (nontestng/* and tck/*) will also be compiled (which are unnecessary compiling). In this patch: Non-test files ("libraries") are moved to "lib" directory; Real tests previously under "test" dir are moved to test/jdk/, and tests under "tck" dir are moved to test/tck/; test/jdk/ and test/tck/ each has TEST.properties with def: TestNG.dirs = . lib.dirs = /java/time/lib /lib/testlibrary MockIOExceptionAppendable.java is not used anywhere, removed. MockSimplePeriod.java previously exist in both "tck" and "test", now it is under "lib". With this change, non-test file then won't be run as testng test and no unnecessary compiling. bug: https://bugs.openjdk.java.net/browse/JDK-8173411 webrev: http://cr.openjdk.java.net/~amlu/8173411/webrev.00/ Thanks, Amy
Hi Amy, I'm not convinced this is the right move. Moving parts of tests further from the test is not helpful from a maintenance point of view. I'm not concerned about compilation time. Its not significant. For example, AbstractDateTimeTest has @Test directives and is not just library code. The files in the 'tck' hierarchy are intended to be self sufficient and a direct mirror or the tests in the JCK. Please provide a more compelling reason for the change. Thanks, Roger On 10/10/2017 11:30 PM, Amy Lu wrote:
Please review the patch to change TestNG.dirs and lib.dirs (def and file structure) for test/jdk/java/time.
test/jdk/java/time contains three sets of tests: nontestng/ tck/ test/
Tests from directory "tck" and "test" are testng tests with properties: TestNG.dirs = .. lib.dirs = ../../../lib/testlibrary lib.build = jdk.testlibrary.RandomFactory
But not all files under "TestNG.dirs" are real tests, some of them are "libraries" thus should not be put under "TestNG.dirs" (thus be run as testng test). Moreover, due to this def, when one runs tests from "test" directory, extra files (nontestng/* and tck/*) will also be compiled (which are unnecessary compiling).
In this patch: Non-test files ("libraries") are moved to "lib" directory; Real tests previously under "test" dir are moved to test/jdk/, and tests under "tck" dir are moved to test/tck/; test/jdk/ and test/tck/ each has TEST.properties with def: TestNG.dirs = . lib.dirs = /java/time/lib /lib/testlibrary MockIOExceptionAppendable.java is not used anywhere, removed. MockSimplePeriod.java previously exist in both "tck" and "test", now it is under "lib".
With this change, non-test file then won't be run as testng test and no unnecessary compiling.
bug: https://bugs.openjdk.java.net/browse/JDK-8173411 webrev: http://cr.openjdk.java.net/~amlu/8173411/webrev.00/
Thanks, Amy
non-test be run as a test, the result may lead to confusion. For example, if look at MockSimplePeriod.java test result, it shows: =============================================== java/time/test/java/time/MockSimplePeriod.java Total tests run: 0, Failures: 0, Skips: 0 =============================================== Without looking into the detail of the test code, one may raise question why this test has 0 testcase. If this is not a concern, I can withdraw this change request. Thanks, Amy On 10/13/17 1:06 AM, Roger Riggs wrote:
Hi Amy,
I'm not convinced this is the right move. Moving parts of tests further from the test is not helpful from a maintenance point of view. I'm not concerned about compilation time. Its not significant.
For example, AbstractDateTimeTest has @Test directives and is not just library code.
The files in the 'tck' hierarchy are intended to be self sufficient and a direct mirror or the tests in the JCK.
Please provide a more compelling reason for the change.
Thanks, Roger
On 10/10/2017 11:30 PM, Amy Lu wrote:
Please review the patch to change TestNG.dirs and lib.dirs (def and file structure) for test/jdk/java/time.
test/jdk/java/time contains three sets of tests: nontestng/ tck/ test/
Tests from directory "tck" and "test" are testng tests with properties: TestNG.dirs = .. lib.dirs = ../../../lib/testlibrary lib.build = jdk.testlibrary.RandomFactory
But not all files under "TestNG.dirs" are real tests, some of them are "libraries" thus should not be put under "TestNG.dirs" (thus be run as testng test). Moreover, due to this def, when one runs tests from "test" directory, extra files (nontestng/* and tck/*) will also be compiled (which are unnecessary compiling).
In this patch: Non-test files ("libraries") are moved to "lib" directory; Real tests previously under "test" dir are moved to test/jdk/, and tests under "tck" dir are moved to test/tck/; test/jdk/ and test/tck/ each has TEST.properties with def: TestNG.dirs = . lib.dirs = /java/time/lib /lib/testlibrary MockIOExceptionAppendable.java is not used anywhere, removed. MockSimplePeriod.java previously exist in both "tck" and "test", now it is under "lib".
With this change, non-test file then won't be run as testng test and no unnecessary compiling.
bug: https://bugs.openjdk.java.net/browse/JDK-8173411 webrev: http://cr.openjdk.java.net/~amlu/8173411/webrev.00/
Thanks, Amy
FWIW, this is a side-effect of taking jtreg having to take some corner cases into account when determining what are "jtreg" tests in a hierarchy of TestNG tests. The underlying problem is that there is no easy reliable way to determine if a source file contains test cases or if it is just a library file. It is not enough to use simple heuristics like scanning the source for @Test annotations, because of the variety of dynamic ways that tests can be found when precompiling all the files and then running them all together with TestNG itself. If we accepted some restrictions, or some guidelines, on marking tests as "jtreg-tests", then we could avoid issues like this, but there was resistance to such a proposal when TestNG support was added to jtreg. -- Jon On 10/12/17 10:39 PM, Amy Lu wrote:
non-test be run as a test, the result may lead to confusion.
For example, if look at MockSimplePeriod.java test result, it shows: =============================================== java/time/test/java/time/MockSimplePeriod.java Total tests run: 0, Failures: 0, Skips: 0 ===============================================
Without looking into the detail of the test code, one may raise question why this test has 0 testcase.
If this is not a concern, I can withdraw this change request.
Thanks, Amy
On 10/13/17 1:06 AM, Roger Riggs wrote:
Hi Amy,
I'm not convinced this is the right move. Moving parts of tests further from the test is not helpful from a maintenance point of view. I'm not concerned about compilation time. Its not significant.
For example, AbstractDateTimeTest has @Test directives and is not just library code.
The files in the 'tck' hierarchy are intended to be self sufficient and a direct mirror or the tests in the JCK.
Please provide a more compelling reason for the change.
Thanks, Roger
On 10/10/2017 11:30 PM, Amy Lu wrote:
Please review the patch to change TestNG.dirs and lib.dirs (def and file structure) for test/jdk/java/time.
test/jdk/java/time contains three sets of tests: nontestng/ tck/ test/
Tests from directory "tck" and "test" are testng tests with properties: TestNG.dirs = .. lib.dirs = ../../../lib/testlibrary lib.build = jdk.testlibrary.RandomFactory
But not all files under "TestNG.dirs" are real tests, some of them are "libraries" thus should not be put under "TestNG.dirs" (thus be run as testng test). Moreover, due to this def, when one runs tests from "test" directory, extra files (nontestng/* and tck/*) will also be compiled (which are unnecessary compiling).
In this patch: Non-test files ("libraries") are moved to "lib" directory; Real tests previously under "test" dir are moved to test/jdk/, and tests under "tck" dir are moved to test/tck/; test/jdk/ and test/tck/ each has TEST.properties with def: TestNG.dirs = . lib.dirs = /java/time/lib /lib/testlibrary MockIOExceptionAppendable.java is not used anywhere, removed. MockSimplePeriod.java previously exist in both "tck" and "test", now it is under "lib".
With this change, non-test file then won't be run as testng test and no unnecessary compiling.
bug: https://bugs.openjdk.java.net/browse/JDK-8173411 webrev: http://cr.openjdk.java.net/~amlu/8173411/webrev.00/
Thanks, Amy
Hi Amy, I don't see a problem with the cases of 'Total tests run: 0'. It is very explicit, no tests were selected. Testng has a number of dynamic mechanisms for test selection such that even files in the test directories might not have any tests to run. As Jon pointed out that's just part of TestNG. Does this throw off the jtreg reports of tests run vs failures? I haven't seen that. I'd leave well enough alone. Thanks, Roger On 10/13/2017 1:39 AM, Amy Lu wrote:
non-test be run as a test, the result may lead to confusion.
For example, if look at MockSimplePeriod.java test result, it shows: =============================================== java/time/test/java/time/MockSimplePeriod.java Total tests run: 0, Failures: 0, Skips: 0 ===============================================
Without looking into the detail of the test code, one may raise question why this test has 0 testcase.
If this is not a concern, I can withdraw this change request.
Thanks, Amy
On 10/13/17 1:06 AM, Roger Riggs wrote:
Hi Amy,
I'm not convinced this is the right move. Moving parts of tests further from the test is not helpful from a maintenance point of view. I'm not concerned about compilation time. Its not significant.
For example, AbstractDateTimeTest has @Test directives and is not just library code.
The files in the 'tck' hierarchy are intended to be self sufficient and a direct mirror or the tests in the JCK.
Please provide a more compelling reason for the change.
Thanks, Roger
On 10/10/2017 11:30 PM, Amy Lu wrote:
Please review the patch to change TestNG.dirs and lib.dirs (def and file structure) for test/jdk/java/time.
test/jdk/java/time contains three sets of tests: nontestng/ tck/ test/
Tests from directory "tck" and "test" are testng tests with properties: TestNG.dirs = .. lib.dirs = ../../../lib/testlibrary lib.build = jdk.testlibrary.RandomFactory
But not all files under "TestNG.dirs" are real tests, some of them are "libraries" thus should not be put under "TestNG.dirs" (thus be run as testng test). Moreover, due to this def, when one runs tests from "test" directory, extra files (nontestng/* and tck/*) will also be compiled (which are unnecessary compiling).
In this patch: Non-test files ("libraries") are moved to "lib" directory; Real tests previously under "test" dir are moved to test/jdk/, and tests under "tck" dir are moved to test/tck/; test/jdk/ and test/tck/ each has TEST.properties with def: TestNG.dirs = . lib.dirs = /java/time/lib /lib/testlibrary MockIOExceptionAppendable.java is not used anywhere, removed. MockSimplePeriod.java previously exist in both "tck" and "test", now it is under "lib".
With this change, non-test file then won't be run as testng test and no unnecessary compiling.
bug: https://bugs.openjdk.java.net/browse/JDK-8173411 webrev: http://cr.openjdk.java.net/~amlu/8173411/webrev.00/
Thanks, Amy
Thank you Roger and Jon for the comments! Change request withdrawn :-) Thanks, Amy On 10/14/17 3:04 AM, Roger Riggs wrote:
Hi Amy,
I don't see a problem with the cases of 'Total tests run: 0'. It is very explicit, no tests were selected. Testng has a number of dynamic mechanisms for test selection such that even files in the test directories might not have any tests to run. As Jon pointed out that's just part of TestNG.
Does this throw off the jtreg reports of tests run vs failures? I haven't seen that.
I'd leave well enough alone.
Thanks, Roger
On 10/13/2017 1:39 AM, Amy Lu wrote:
non-test be run as a test, the result may lead to confusion.
For example, if look at MockSimplePeriod.java test result, it shows: =============================================== java/time/test/java/time/MockSimplePeriod.java Total tests run: 0, Failures: 0, Skips: 0 ===============================================
Without looking into the detail of the test code, one may raise question why this test has 0 testcase.
If this is not a concern, I can withdraw this change request.
Thanks, Amy
On 10/13/17 1:06 AM, Roger Riggs wrote:
Hi Amy,
I'm not convinced this is the right move. Moving parts of tests further from the test is not helpful from a maintenance point of view. I'm not concerned about compilation time. Its not significant.
For example, AbstractDateTimeTest has @Test directives and is not just library code.
The files in the 'tck' hierarchy are intended to be self sufficient and a direct mirror or the tests in the JCK.
Please provide a more compelling reason for the change.
Thanks, Roger
On 10/10/2017 11:30 PM, Amy Lu wrote:
Please review the patch to change TestNG.dirs and lib.dirs (def and file structure) for test/jdk/java/time.
test/jdk/java/time contains three sets of tests: nontestng/ tck/ test/
Tests from directory "tck" and "test" are testng tests with properties: TestNG.dirs = .. lib.dirs = ../../../lib/testlibrary lib.build = jdk.testlibrary.RandomFactory
But not all files under "TestNG.dirs" are real tests, some of them are "libraries" thus should not be put under "TestNG.dirs" (thus be run as testng test). Moreover, due to this def, when one runs tests from "test" directory, extra files (nontestng/* and tck/*) will also be compiled (which are unnecessary compiling).
In this patch: Non-test files ("libraries") are moved to "lib" directory; Real tests previously under "test" dir are moved to test/jdk/, and tests under "tck" dir are moved to test/tck/; test/jdk/ and test/tck/ each has TEST.properties with def: TestNG.dirs = . lib.dirs = /java/time/lib /lib/testlibrary MockIOExceptionAppendable.java is not used anywhere, removed. MockSimplePeriod.java previously exist in both "tck" and "test", now it is under "lib".
With this change, non-test file then won't be run as testng test and no unnecessary compiling.
bug: https://bugs.openjdk.java.net/browse/JDK-8173411 webrev: http://cr.openjdk.java.net/~amlu/8173411/webrev.00/
Thanks, Amy
participants (3)
-
Amy Lu
-
Jonathan Gibbons
-
Roger Riggs