Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixing file leaks in tests (preadapting to proposed TemporaryDirectoryAllocator.dispose change) #8006

Merged
merged 9 commits into from
May 21, 2023
Merged

Fixing file leaks in tests (preadapting to proposed TemporaryDirectoryAllocator.dispose change) #8006

merged 9 commits into from
May 21, 2023

Conversation

jglick
Copy link
Member

@jglick jglick commented May 16, 2023 •
edited by gitpod-io bot
Loading

Fixing some issues in tests that could result in file handles being left open, especially on Windows.

Testing done

Ran tests against jenkinsci/jenkins-test-harness#198 which is stricter about cleanup.

Proposed changelog entries

  • N/A

Maintainer checklist

Before the changes are marked as ready-for-merge:

  • There are at least two (2) approvals for the pull request and no outstanding requests for change.
  • Conversations in the pull request are over, or it is explicit that a reviewer is not blocking the change.
  • Changelog entries in the pull request title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood.
  • Proper changelog labels are set so that the changelog can be generated automatically.
  • If the change needs additional upgrade steps from users, the upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).
  • If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

@jglick
Copy link
Member Author

jglick commented May 16, 2023 

[ERROR] hudson.LauncherTest.remotable  Time elapsed: 4.281 s  <<< ERROR!
java.nio.file.FileSystemException: C:\Jenkins\workspace\Core_jenkins_PR-8006\test\target\j h12573638483276859145\log: The process cannot access the file because it is being used by another process
	at java.base/sun.nio.fs.WindowsException.translateToIOException(WindowsException.java:92)
	at java.base/sun.nio.fs.WindowsException.rethrowAsIOException(WindowsException.java:103)
	at java.base/sun.nio.fs.WindowsException.rethrowAsIOException(WindowsException.java:108)
	at java.base/sun.nio.fs.WindowsFileSystemProvider.implDelete(WindowsFileSystemProvider.java:275)
	at java.base/sun.nio.fs.AbstractFileSystemProvider.deleteIfExists(AbstractFileSystemProvider.java:110)
	at java.base/java.nio.file.Files.deleteIfExists(Files.java:1191)
	at org.jvnet.hudson.test.TemporaryDirectoryAllocator.delete(TemporaryDirectoryAllocator.java:135)
	at org.jvnet.hudson.test.TemporaryDirectoryAllocator.delete(TemporaryDirectoryAllocator.java:130)
	at org.jvnet.hudson.test.TemporaryDirectoryAllocator.dispose(TemporaryDirectoryAllocator.java:98)
	at org.jvnet.hudson.test.TestEnvironment.dispose(TestEnvironment.java:84)
	at …

smells like a genuine test bug.

@jglick
Copy link
Member Author

jglick commented May 16, 2023

https://github.com/jenkinsci/jenkins/pull/8006/checks?check_run_id=13524678256 also probably a test bug; does not wait for all builds it triggered to complete.

@github-actions github-actions bot added the unresolved-merge-conflict There is a merge conflict with the target branch. label May 17, 2023
@github-actions

This comment was marked as outdated.

Sign in to view
@github-actions github-actions bot removed the unresolved-merge-conflict There is a merge conflict with the target branch. label May 17, 2023
@jglick jglick mentioned this pull request May 18, 2023
Merged
jglick
jglick commented May 18, 2023
View reviewed changes
test/src/test/java/hudson/LauncherTest.java
} else {
ps.cmds("echo", "hello");
try (var listener = new RemotableBuildListener(log)) {
Launcher.ProcStarter ps = rule.createOnlineSlave().createLauncher(listener).launch();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(hide whitespace to review)

@jglick jglick changed the title Testing TemporaryDirectoryAllocator.dispose change Fixing file leaks in tests (preadapting to proposed TemporaryDirectoryAllocator.dispose change) May 18, 2023
@jglick jglick requested a review from jtnord May 18, 2023 11:33
@jglick jglick marked this pull request as ready for review May 18, 2023 11:34
jtnord

This comment was marked as resolved.

Sign in to view

NotMyFault
NotMyFault approved these changes May 20, 2023
View reviewed changes
Copy link
Member

@NotMyFault NotMyFault left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/label ready-for-merge

This PR is now ready for merge. We will merge it after ~24 hours if there is no negative feedback.
Please see the merge process documentation for more information about the merge process.
Thanks!

@comment-ops-bot comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label May 20, 2023
@NotMyFault NotMyFault added the skip-changelog Should not be shown in the changelog label May 20, 2023
@NotMyFault NotMyFault merged commit 6531cde into jenkinsci:master May 21, 2023
@jglick jglick deleted the TemporaryDirectoryAllocator.dispose branch May 22, 2023 15:59
@jglick jglick mentioned this pull request Jul 12, 2023
Merged
@basil
Copy link
Member

basil commented Nov 29, 2023

LauncherTest still flaky on Windows: https://github.com/jenkinsci/jenkins/pull/8728/checks?check_run_id=19150437978

@jglick
Copy link
Member Author

jglick commented Nov 29, 2023

Thanks, will take a look soon.

@jglick jglick mentioned this pull request Dec 4, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NotMyFault NotMyFault NotMyFault approved these changes

@jtnord jtnord jtnord approved these changes

Assignees
No one assigned
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback skip-changelog Should not be shown in the changelog
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jglick @basil @jtnord @NotMyFault