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

RealJenkinsRule: add ability to check if Jenkins fails to boot. #451

Merged
merged 9 commits into from
Jul 12, 2022

Conversation

joseblas
Copy link
Contributor

@joseblas joseblas commented Jul 4, 2022

When executing some tests that are expecting Jenkins not to start the timeout is never executed. Changing to before the status check makes that possible, also makes more sense as the time starts to count when the server starts, not when is up and running.

No new tests required as the functionality remains the same.

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

@jtnord jtnord requested a review from jglick July 5, 2022 09:37
@jtnord
Copy link
Member

jtnord commented Jul 5, 2022

This seems incorrect to me - if Jenkins is never going to start up then the process should have exited or there should be a boot failure.

If Jenkins has failed to start then the boot-failure.txt file should exist and this should probably be used to exit early rather than a timeout which will always lead to the test taking longer than expected? if the java process has failed to start the proc should have exited so checking that it has not in fact terminated would also be expected.

@joseblas
Copy link
Contributor Author

joseblas commented Jul 5, 2022

Indeed, didn't remember that file. Thanks @jtnord

@jglick jglick changed the title changing timeout positionin the code Apply timeout as soon as process is launched Jul 6, 2022
@jglick jglick changed the title Apply timeout as soon as process is launched RealJenkinsRule: apply timeout as soon as process is launched Jul 6, 2022
@jglick
Copy link
Member

jglick commented Jul 6, 2022

No new tests required as the functionality remains the same.

That is not true, right? The whole point is that this patch (possibly augmented by a more eager check for boot-failure.txt) would enable you to write a test that expects Jenkins not to start which would fail before and pass now. Or am I missing something? Hard to evaluate without a concrete example of what you are trying to do (preferably in the form of a new test case).

@jglick jglick added the bug label Jul 6, 2022
@joseblas
Copy link
Contributor Author

joseblas commented Jul 7, 2022

@jglick no tests needed if the change were only to move up the timeout. With current changes it needs them for sure.

@joseblas joseblas requested a review from jglick July 7, 2022 13:00
@jglick jglick requested a review from jtnord July 7, 2022 15:00
@jglick
Copy link
Member

jglick commented Jul 7, 2022

no tests needed if the change were only to move up the timeout

I am not sure that is true. I am still unclear on what problem you are solving exactly—the original motivation. Unit tests are fine (though I would prefer simple Java code rather than mocks) but there is no test of RJR showing what was broken before which is now fixed. Specifically what does

tests that are expecting Jenkins not to start

mean?

@joseblas
Copy link
Contributor Author

joseblas commented Jul 7, 2022

When RJR cannot start (for whatever reason) it hangs up in the while(true) loop.
First attempt was to put timeout up before the loop, which is flaky as @jtnord mentioned.
So, I added this change, if the status is providing a 500 (instead of classic not ready - 503) then there is an issue starting RJR.
This corner case if what this PR wants to fix.

With the UT I demonstrate that when a 500 is coming the loop is broken and an exception is thrown.

@joseblas joseblas changed the title RealJenkinsRule: apply timeout as soon as process is launched RealJenkinsRule: add ability to check if Jenkins fails to boot. Jul 8, 2022
@joseblas joseblas requested a review from jtnord July 8, 2022 09:39
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Looks close. Some minor suggestions.

@jglick jglick added enhancement and removed bug labels Jul 8, 2022
@joseblas joseblas requested a review from jglick July 11, 2022 11:09
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Some trivia comments, but I think this is good to go; @jtnord do you care to review again?

@jtnord jtnord merged commit 6d4e97d into jenkinsci:master Jul 12, 2022
@joseblas joseblas deleted the changing-timeout branch July 12, 2022 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants