You are right, restoring system properties to their original state is the right thing to do. While the current agent is "othervm", and therefore is sufficiently isolated, this might change in the future.
As for that "TODO" comment. I left the comment but stripped the "TODO" semantics off it. -Pavel > On 30 Apr 2020, at 20:48, Jonathan Gibbons <[email protected]> > wrote: > > Generally very good. > > As a suggestion for an optional improvement, the new test is OK, but given > the use of system properties, better test hygiene would be to set a new > properties object, and remove it when the test is complete. This could be > done in a try-finally block around the javadoc invocation. > > Given this comment in the test: > > >> >> 146 /* TODO: Ideally, these system properties should be passed to >> the `javadoc` method below. >> 147 However, as of today there's no such option, so for now this >> is hardcoded */ >> > > I don't anticipate adding support for system properties into JavadocTester > since this test is a specific corner case where you can't pass values to the > doclet via options, which would be a better methodology for any other puppet > doclet being used to test non-option features. What you have done is more > than good enough to pass out-of-band data, and the suggestion to temporarily > install/remove a new properties would be even better. Best of all would be > to be able to pass a configured instance of a doclet into JavadocTester and > from there into an instance of the javadoc tool via its API ... but that last > step is not yet available. > > -- Jon > > On 4/29/20 11:02 AM, Pavel Rappo wrote: >> Hello, >> >> Please review the change for >> https://bugs.openjdk.java.net/browse/JDK-8224613 >> >> >> >> http://cr.openjdk.java.net/~prappo/8224613/webrev.00/ >> >> >> This addresses a scenario in which a doclet failure was previously >> overlooked. More specifically, if a doclet returned false after processing >> one or more of its options while otherwise staying silent, the tool >> continued its execution. >> >> -Pavel >> >>
