On 12/6/19 12:51 PM, Alexey Semenyuk wrote:
On 12/5/2019 8:44 PM, Alexander Matveev wrote:
Hi Alexey,
1) Remove this file:
test/jdk/tools/jpackage/helpers/jdk/jpackage/test/JavaTool.java.rej
Done. webrev updated in place.
2) Agree with Phil, they probably should be pushed as two separate bugs.
Decoupling is not straightforward unfortunately: tests that cover
additional launchers functionality contain fixes for both [1] and [2]
CRs.
So the synopsis reads like this is all about updating tests, and your
email says
"go read the bugs" and nothing more.
I think most people like to see a description of the problem and the
proposed solution
written in the review request.
I think the source code updates are what you need to highlight.
And I don't think you have explained why it is so hard.
Saying is is hard is not the same thing as explaining why.
I think you should decouple unless you can provide that explanation.
And definitely you need to provide explanation of the fix etc.
-phil.
If there would be no strong arguments against pushing the fixes in a
single patch, I'd rather avoid decoupling.
3) Do you know how to run installer tests with new changes? It is not
clear from code.
Fix for [1] moved code to install packages produced by jpackage from
manage_packages.sh script into jtreg helper classes.
If you run tests locally with test_jpackage.sh script, nothing will
change for you.
- Alexey
Changes itself looks fine.
Thanks,
Alexander
On 12/5/2019 5:33 PM, Philip Race wrote:
I don't understand the relationship between these two bugs.
-phil
On 12/5/19, 2:47 PM, Alexey Semenyuk wrote:
Please review fixes for [1] and [2]. Both target jpackage tool.
The webrev is at [3].
[1] https://bugs.openjdk.java.net/browse/JDK-8233270
[2] https://bugs.openjdk.java.net/browse/JDK-8230933
[3] http://cr.openjdk.java.net/~asemenyuk/8233270/webrev.00/
