Hi Hamlin, I'm going to add explicit @build for jdk.test.lib.** classes in jdk tests which are in :tier[1-3].
unfortunately this approach does not work if a test depends on a library indirectly, e.g. you might have a couple of tests which share some code (but not from /test/lib or /jdk/test/lib/testlibrary) and this code depends on a testlibrary, so you will need to analyze test classes files, which is not much different from analyzing them to get all explicit @build. since your approach works only if all testlibrary classes are in a package, we will need to update lots of test which use classes from default package, which makes it a bit more painful. but the main disadvantage is the assumption that all tests must follow this rule, and if one test does not follow it, this test might pass, while other innocent tests will fail. if we were considering approaches w/ such flaw, I'd strongly recommend to simply remove all @build actions (except needed due to reflection accesses) and have the straightforward rule to follow and one-liner to check this rule. -- Igor > On Jun 8, 2017, at 7:58 PM, Hamlin Li <huaming...@oracle.com> wrote: > > Hi Igor, > > I'm coping Jon as it needs Jon's comments. > > > Thank you for doing such a great refactoring, I believe it will make tests > run more stable. > > I saw you are adding explicit @build to lots of test, are you going to clean > up all tests to add explicit @build? If the answer is "yes", then I have a > possible simple solution for you to consider. > > Steps: > 1. refactor all test library classes in unnamed packages to named package. > 2. just need to add explicit @build for every "import x.y.z;" in a test, > means you only need to add @build for every test lib class directly used by > your test, lib classes' dependency is not needed. e.g. there is a test > containing only "import jdk.test.lib.process.ProcessTools;", then only need > to add "@build jdk.test.lib.process.ProcessTools", and no @build is needed to > be added for jdk.test.lib.process.StreamPumper, jdk.test.lib.JDKToolFinder > and jdk.test.lib.Utils and further dependency if they have. > This solution is based on the situation that all tests will be added @build > for all test lib classes they directly used, that means no test is allow to > just add "@library xxx". > > The advantage of this solution is: > 1. the rule is simple to follow, reduce the pain for engineer writing the > tests. > 2. it might be possible to do it automatically, and have a tool to monitor > all checked-in test code by this rule; > 3. no need to refactor the test lib too much, e.g. no need to remove the > dependency between test lib packages; > > > Of course, Jon's original strict solution is absolutely correct: to add > @build for all test lib classes even if for the implicitly dependent classes. > Jon's strict solution is for a more complicated situation where some tests > are using explicit @build, but others are not, in this mixed situation my > solution will NOT work. But as you're cleaning up all the tests, I think it's > not necessary to follow such a strict rule. > > I might miss something, please correct me then. > > Thank you > > -Hamlin > > On 2017/6/8 15:20, Igor Ignatyev wrote: >> http://cr.openjdk.java.net/~iignatyev//8181761/webrev.00/index.html >> <http://cr.openjdk.java.net/~iignatyev//8181761/webrev.00/index.html> >>> 432 lines changed: 404 ins; 1 del; 27 mod; >> Hi all, >> >> could you please review this changeset which adds explicit @build actions to >> tier2 jdk tests? other tests will be updated by the corresponding sub-tasks >> of 8181758[1]. >> >> webrev: http://cr.openjdk.java.net/~iignatyev//8181761/webrev.00/index.html >> <http://cr.openjdk.java.net/~iignatyev//8181761/webrev.00/index.html> >> jbs: https://bugs.openjdk.java.net/browse/JDK-8181761 >> <https://bugs.openjdk.java.net/browse/JDK-8181761> >> testing: :tier2 (in progress) >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8181758 >> <https://bugs.openjdk.java.net/browse/JDK-8181758> >> >> Thanks, >> -- Igor >
