RFR: JDK-8233270: Add support to jtreg helpers to unpack packages
Alexey Semenyuk
alexey.semenyuk at oracle.com
Sat Dec 14 00:47:55 UTC 2019
Phil,
Let me provide more details on what is going on here.
JDK-8230933 - is the fix of jpackage code assigning icons to
main/additional launchers.
JDK-8233270 - is primarily the enhancement of jpackage test helpers to
provide functionality needed for automated testing of the fix of
JDK-8230933.
In order to test JDK-8230933 fix it is necessary to access contents of
packages created by jpackage. This is normally done when packages are
installed. However our test environment doesn't allow installing of
packages. The alternative is to extract contents of packages without
installing them.
So in order to implement jtreg tests for JDK-8230933 fix the
functionality to unpack packages was needed. JDK-8233270 was filed to
track this enhancement of jpackage test helpers. Work on JDK-8233270
resulted in quite a big refactoring of tests and test helpers of jpackage:
Changes to jtreg test helpers:
- added support to unpack .dmg, .deb, .rpm, .msi packages.
- added AdditionalLauncher helper class to test additional launchers
functionality of jpackage.
- added LauncherIconVerifier helper class to test how jpackage assigns
icons to launchers (main and additional).
- numerous minor code improvements.
- bugfixes.
Changes to jtreg tests:
- all temporary directories/files created during tests runs that can
help to investigate test failures are created in a way they are deleted
only when tests pass and not when tests exit.
- removed unused files from test\jdk\tools\jpackage\apps\com.other folder.
- refactored additional launchers jtreg tests based on the old test
helpers inherited from javapackager to use the new test helpers.
Logically JDK-8230933 and JDK-8233270 issues are unrelated, they got
tangled because JDK-8230933 came first and then JDK-8233270 followed.
The proper way to address them was put on hold JDK-8230933 fix and
implement JDK-8233270 first. Then work on JDK-8230933 fix on top of
JDK-8233270. However the work on these two issues was carried out in the
sandbox with the assumption they both would be a part of cumulative
jpackage patch rolled in jdk14. Keeping them separated was not a priority.
Hope this clarifies what is in the webrev and why JDK-8230933 and
JDK-8233270 ended up in the same patch.
- Alexey
On 12/6/2019 3:59 PM, Phil Race wrote:
>
>
> 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/
>>>>>
>>>
>>
>
More information about the core-libs-dev
mailing list