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

[JENKINS-71406] Flake in ExecutorTest.apiPermissions caused by builds left running #8157

Closed
wants to merge 4 commits into from

Conversation

jglick
Copy link
Member

@jglick jglick commented Jun 16, 2023

See JENKINS-71406. Similar to #8115.

Testing done

Ran locally, confirmed that it was aborting a few builds.

Proposed changelog entries

  • N/A

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

Maintainer checklist

Preview Give feedback

Copy link
Member

@basil basil left a comment

Choose a reason for hiding this comment

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

Are we confident that ExecutorTest is the only case of this problem?

@jglick
Copy link
Member Author

jglick commented Jun 16, 2023

It was the only such failure I could see in the currently available history. If other buggy test cases are subsequently discovered, they can be fixed as well.

Copy link
Member

@basil basil left a comment

Choose a reason for hiding this comment

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

Not the right fix I think. Fails to diagnose the essential problem: that ExecutorTest#startBlockingBuild (which is invoked not just from ExecutorTest) returns a build for whose completion consumers are obligated to wait.

Copy link
Member

@basil basil left a comment

Choose a reason for hiding this comment

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

More transparent (and consistent with existing tests) to stop the builds in the same scope as they were started.

@basil
Copy link
Member

basil commented Jun 28, 2023

This is still inconsistent with the pattern used in existing tests in core and plugins that simply stop any running builds that are in scope at the end of the test (not in a finally block and not by iterating over all builds). An approach that is consistent with the pattern used in existing tests is #8158.

I am not necessarily against switching to a new approach, but I think any possible new approach should be done consistently throughout this test suite at least, while also allowing plugin tests to use the same approach. I think that would likely take the form of opting in (or opting out?) of generic cleanup logic in the Jenkins test harness (JTH) rather than writing a stopRunningBuilds() method in every repository that contains tests.

@jglick
Copy link
Member Author

jglick commented Jun 28, 2023

Did not see #8158; that seems more straightforward.

@jglick jglick closed this Jun 28, 2023
@jglick jglick deleted the ExecutorTest-JENKINS-71406 branch June 28, 2023 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants