-
Notifications
You must be signed in to change notification settings - Fork 126
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
Improve InboundAgentRule
#493
Conversation
So far this has passed a regular Jenkins core test run as well as running all |
public static class Options { | ||
// TODO Java 14+ use records | ||
|
||
@CheckForNull private String name; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
personal preference would be to move the annotation above it, it looks strange to me as it's in the same grouping of variables and feels mis-aligned. (feel free to ignore)
@CheckForNull private String name; | |
@CheckForNull | |
private String name; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
google-java-format
puts them on the same line if the whole line would be within the margin, and I have seen another argument that they should be on the same line because type annotations should be close to their corresponding type. I do not care much either way.
* <p>Instances of {@link Builder} are created by calling {@link | ||
* InboundAgentRule.Options#newBuilder}. | ||
*/ | ||
public interface Builder { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would you be able to explain why you went for an interface for a builder? It's not a pattern I'm familiar with (of course there's many slightly different ways to do builders).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just being paranoid about not exposing internal implementation details to consumers I suppose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also struggled to understand this (looking into #514 (comment)). Conventionally builders would be concrete final
classes. AFAICT the only implementation is in fact BuilderImpl
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something else I found confusing is that Options
has setters as well as a builder; conventionally only a builder would have mutable state, and the built class would use final
fields. Is there some reason these would be called after BuilderImpl.build
has returned?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you do not like it, then change it.
start(r, name); | ||
// SlaveComputer#_connect runs asynchronously. Wait for it to finish for a more deterministic test. | ||
while (s.toComputer().getOfflineCause() == null) { | ||
Thread.sleep(100); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a chance this never happens and we end up with an infinite loop? I guess the test timeout would kick in eventually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, just like in a half-dozen other methods in JenkinsRule
.
Various improvements to
InboundAgentRule
:SlaveComputer#_connect
to finish after adding the agent but before starting itThe motivation behind this is to enable most tests in Jenkins core to be converted to this rule. That way flakiness fixes can be implemented in one place and apply to all tests, which isn't possible with the copypasta we have today.
Once I get an incremental I'll post a core PR demonstrating how this is used.