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.

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

Stop swallowing exceptions that happen in TestEnvironment#dispose() #166

Merged
merged 1 commit into from
Oct 8, 2019

Conversation

slonopotamus
Copy link
Contributor

@slonopotamus slonopotamus commented Sep 23, 2019

This commit is excepted to reveal bogus tests that do not properly shutdown,
especially on Windows where directory cannot be removed if there are any running processes that use it as a working directory.

This commit is excepted to reveal bogus tests that do not properly shutdown,
especially on Windows where directory cannot be removed if there are any running processes
that use it as a working directory.
@oleg-nenashev oleg-nenashev changed the title Stop swallowing exceptions that happen in TestEnvironment.dispose Stop swallowing exceptions that happen in TestEnvironment#dispose() Oct 8, 2019
@oleg-nenashev oleg-nenashev merged commit 07275e8 into jenkinsci:master Oct 8, 2019
@slonopotamus slonopotamus deleted the dispose-exceptions branch October 8, 2019 16:14
@slonopotamus
Copy link
Contributor Author

slonopotamus commented Dec 16, 2019

jenkinsci/git-plugin#798 is a nice case of a bug that was found thanks to this PR + Windows CI.

slonopotamus added a commit to slonopotamus/subversion-plugin that referenced this pull request Dec 17, 2019
This commit fixes file descriptor leak that is detected by jenkinsci/jenkins-test-harness#166
@slonopotamus
Copy link
Contributor Author

File descriptor leak in subversion plugin: jenkinsci/subversion-plugin#240
Technically, this is a MapDB bug that got fixed in later MapDB releases, but subversion plugin depended on ancient version.

@slonopotamus
Copy link
Contributor Author

slonopotamus commented Dec 17, 2019

Attempt to upgrade workflow-cps-plugin to JTH with current fix discovered a bug in JTH itself: jenkinsci/workflow-cps-plugin#345 o_O

Similar issue in jenkinsci/lockable-resources-plugin#170

slonopotamus added a commit to slonopotamus/lockable-resources-plugin that referenced this pull request Dec 17, 2019
This commit fixes test failure with modern JTH detected by jenkinsci/jenkins-test-harness#166
@slonopotamus
Copy link
Contributor Author

Bug in workflow-support caused by dependency on old workflow-job: jenkinsci/workflow-support-plugin#99.

@slonopotamus
Copy link
Contributor Author

Minor leaks in workflow-basic-steps-plugin tests: jenkinsci/workflow-basic-steps-plugin#103

@basil
Copy link
Member

basil commented Dec 18, 2019

I filed PRs to update pipeline-build-step, pipeline-githubnotify-step, pipeline-graph-analysis, pipeline-input-step, pipeline-milestone-step, pipeline-stage-step, pipeline-utility-steps, workflow-api, and workflow-cps-global-lib. This exposed a few bugs in tests, which I fixed in jenkinsci/pipeline-graph-analysis-plugin#29, jenkinsci/pipeline-input-step-plugin#42, jenkinsci/pipeline-utility-steps-plugin#72, and jenkinsci/workflow-cps-global-lib-plugin#88.

@slonopotamus
Copy link
Contributor Author

Cool! The only plugin that gives me headaches now is lockable-resources: jenkinsci/lockable-resources-plugin#170.

@MarkEWaite
Copy link
Contributor

Thanks very much for enabling this setting and for fixing the bugs that it discovered in the git plugin. Very much appreciated.

I was unwilling to wrestle further with the intermittent GitSCMTest and GitStatusTest build log related failures on ci.jenkins.io from this change. I don't see those failures on the 5 Windows 10 machines that are in my local cluster. Refer to jenkinsci/git-plugin#802 for the workaround which adds a call to JenkinsRule.waitUntilNoActivityUpTo() into GitSCMTest when running on ci.jenkins.io and disables a few tests in GitStatusTest when running on ci.jenkins.io.

Thanks again for this change. A very nice addition!

@slonopotamus
Copy link
Contributor Author

slonopotamus commented Dec 20, 2019

WRT locks on build log: I'm not quite sure what's going on there. When working on jenkinsci/workflow-support-plugin#99, I thought there's an issue in workflow-api/workflow-job plugin. But same solution didn't fix jenkinsci/lockable-resources-plugin#170. This needs more investigation. While sleeping for several seconds might allow tests to pass, I feel like this is just a way to hide a bug. My current understanding is that build log should not be accessed by anything after the build has finished.
And this is really sad that you have to add workarounds for this PR, it is not the solution I initially thought of when was introducing this. I'll spend more time figuring out what prevents lockable-resources from releasing files. Hopefully, this will also shed the light on git-plugin failures.

darxriggs added a commit to darxriggs/support-core-plugin that referenced this pull request Feb 8, 2020
This is to stay up-to-date and use the latest features and fixes.

The newer jenkins-test-harness provided with this change does not
any longer swallow exceptions on test teardown. See
jenkinsci/jenkins-test-harness#166

Therefore some tests have been changed to leave the environment in
a proper state. Otherwise they would fail under Windows because some
Pipeline folders cannot be removed due to open file handles.

The `SemaphoreStep` was not used properly and anyway not required in
most cases. It has to be used correctly which requires 1-2 lines of
code in every test to not leave the environment in a bad state.
The platform-independent `echo` step is sufficient and requires no
extra code. Using no step at all would maybe also be an option.
darxriggs added a commit to darxriggs/support-core-plugin that referenced this pull request Feb 8, 2020
The newer jenkins-test-harness provided with this change does not
any longer swallow exceptions on test teardown. See
jenkinsci/jenkins-test-harness#166

This reveals problems in the implementation and tests. Under Windows
tests failed because some folders could not be removed due to open
file handles.

Fixes:

1) In `BaseFileContent` the `InputStream` was not closed correctly
in every case.

2) The `SemaphoreStep` was not used properly and anyway not required
in most cases. It has to be used correctly which requires 1-2 lines
of code in every test to not leave the environment in a bad state.
The platform-independent `echo` step is sufficient and requires no
extra code. Using no step at all may also be an option.
darxriggs added a commit to darxriggs/support-core-plugin that referenced this pull request Feb 8, 2020
The newer jenkins-test-harness provided with this change does not
any longer swallow exceptions on test teardown. See
jenkinsci/jenkins-test-harness#166

This reveals problems in the implementation and tests. Under Windows
tests failed because some folders could not be removed due to open
file handles.

Fixes:

1) In `BaseFileContent` the `InputStream` was not closed correctly
in every case.

2) The `SemaphoreStep` was not used properly and anyway not required
in most cases. It has to be used correctly which requires 1-2 lines
of code in every test to not leave the environment in a bad state.
The platform-independent `echo` step is sufficient and requires no
extra code. Using no step at all may also be an option.
darxriggs added a commit to darxriggs/support-core-plugin that referenced this pull request Feb 9, 2020
The newer jenkins-test-harness provided with this change does not
any longer swallow exceptions on test teardown. See
jenkinsci/jenkins-test-harness#166

This reveals problems in the implementation and tests. Under Windows
tests failed because some folders could not be removed due to open
file handles.

Fixes:

1) In `BaseFileContent` the `InputStream` was not closed correctly
in every case.

2) The `SemaphoreStep` was not used properly and anyway not required
in most cases. It has to be used correctly which requires 1-2 lines
of code in every test to not leave the environment in a bad state.
The platform-independent `echo` step is sufficient and requires no
extra code. Using no step at all may also be an option.
slonopotamus added a commit to slonopotamus/lockable-resources-plugin that referenced this pull request Feb 27, 2020
This commit fixes test failure with modern JTH detected by jenkinsci/jenkins-test-harness#166
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants