On Mon, 25 Nov 2024 17:00:17 GMT, Roger Riggs <rri...@openjdk.org> wrote:
> To the motivation, please list the tests that would be modified to use this > test feature. Thanks Roges! Yes, I realize this came a bit out of the blue. The test I have been looking at so far include: `test/jdk/java/net/URLClassLoader/RemoveJar.java` This verifies that `URLClassLoader` (via JarURLConnection) when closed also closes the opened JarFile, under various loading scenarios and error conditions. The assertion that the file is closed is done by deleting the file and expecting that to fail. This expectation works only on Windows. `test/jdk/java/net/URLClassLoader/JarLoaderCloseTest.java` Verifies that malformed URLs in the `Class-Path:` Manifest attribute of a JAR does not cause a file descriptor leak for the JAR file. Also relies on delete failing on Windows. `test/jdk/sun/net/www/protocol/jar/FileURLConnectionLeak.java` This verifies that calling `URLConnection::getLastModified` on a `JarURLConnection` does not leak a file descriptor for the underlying FileURLConnection. Also expects Files.delete to fail on Windows. On Linux, it checks files in `/proc/<pid>/fd`, which means it can only assert a closed file, not an open file. If this PR gets approved, I plan to update the above tests in one or more follow-up PRs. I haven't looked too closely outside the java/net area, but expect there are other existing tests which could benefit from this. Presumably there are also current or future tests which would have asserted file open status if a solution was available. > To the feature itself, it seems pretty complicated and invasive, please > expand on the necessity. The goal is to: * Make tests express their intent more clearly. The "delete on windows" idiom requires some explaining via comments. `assertClosed(file)` is more succinct and says what it does. * Allow asserting open status without deleting the file. The need to delte means the assertion can only happen at the end of the test. With `assertOpen` / `assertClosed`, we can do the assert at any point in the test, meaning we can verify that an action opens or closes a file with a much finer granularity. * Allowing asserts to work cross platform. This improves developer ergonomics, since a test can be developed without the need to test every change on multiple OS platforms (typically requiring a CI run). > Note that the initial comment in any PR is copied in every email sent on the > PR and makes it harder to get to the point in subsequent emails. Additional > details can/should be included in separate comments or in the Jira issue. Good point, I moved the details into s separate comment. > To have a clean PR, it would have been a benefit to reviewers to have > squashed the work in progress commits before opening the PR. Thanks, I squashed and force pushed before any code review comments. Hope that helps! > Thanks for developing this approach to a difficult test problem. Thanks for your comments. I know Java agents introduces some complexity, but I was positively surprised by the small number of classes needing instrumentation (just four). These are classes expected to have little churn. I also found the new ClassFile API a pleasure to use. ------------- PR Comment: https://git.openjdk.org/jdk/pull/22343#issuecomment-2498647708
