On Wed, 22 Nov 2023 18:01:21 GMT, Eirik Bjorsnos <d...@openjdk.org> wrote:
>> @eirbjo Yes, as you noticed, the jar file does matter. And the reason I >> suspected it wasn't noticed was because it was in the scenario of running >> without security manager, So may that part of the code wasn't being actively >> executed. >> >> As for the rewrite, it does look good. But would it make more sense to bring >> this change as a separate PR having a own openjdk bug issue # designated to >> reworking of BadFactoryTest.sh for tracking purposes? > >> As for the rewrite, it does look good. But would it make more sense to bring >> this change as a separate PR having a own openjdk bug issue # designated to >> reworking of BadFactoryTest.sh for tracking purposes? > > We have two options: > > - Withdraw this PR, submit a new PR for the rewrite > - Repurpose this PR for the rewrite, updating JBS and PR titles accordingly > > In either case, I can help updating/creating JBS isssues as required. > > I have a slight preference for repurposing, but it's up to you. What do you > prefer? > @eirbjo If it makes sense from perspective of commiters/reviewers, I'll be > happy to integrate the changes here and tweak so that it looks good. Reviewer time is a scarce resource. It would be wasteful to spend review cycles on getting a fix of this `.sh` test integrated now and then immediately follow up with a delete in the rewrite PR. I think we should handle this change in one PR, not two. If you prefer to keep the history of your `.sh` fix documented, we can repurpose this PR, otherwise we start fresh with a new PR for the rewrite. > I also wonder if we have BadFactoryTest.java instead of BadFactoryTest.sh , > will certain references elsewhere need to be adjusted accordingly, in case > these jtreg tests are being referenced in a certain way somewhere. jtreg will pick this up automatically. I did a search of `BadFactoryTest` across the code base, and it is not referenced outside the file itself. ------------- PR Comment: https://git.openjdk.org/jdk/pull/16585#issuecomment-1823271462
