-
Notifications
You must be signed in to change notification settings - Fork 182
Miscellaneous Test Coverage #1460
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
Conversation
| private Headers _add(String key, @NonNull Collection<String> values) { | ||
| if (readOnly) { | ||
| throw new UnsupportedOperationException(); | ||
| } |
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.
This was redundant, all callers already do this.
| else { | ||
| pubSubChunkPrefix = oso.getJetStreamOptions().getPrefix() + rawChunkPrefix; | ||
| pubSubMetaPrefix = oso.getJetStreamOptions().getPrefix() + rawMetaPrefix; | ||
| } |
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.
These variables are unused.
| Validator.validateNotNull(meta, "ObjectMeta"); | ||
| Validator.validateNotNull(meta.getObjectName(), "ObjectMeta name"); | ||
| Validator.validateNotNull(inputStream, "InputStream"); | ||
| Validator.validateNotNull(meta.getObjectMetaOptions(), "Meta Options"); |
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.
intellij reminds me that I now have a nullability annotation that says this getObjectMetaOptions can return null
| if (oi.isLink()) { | ||
| ObjectLink link = oi.getLink(); | ||
| if (link.isBucketLink()) { | ||
| if (link == null || link.isBucketLink()) { |
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.
nullability annotation that says getLink can return null
| if (totalBytes != oi.getSize()) { throw OsGetSizeMismatch.instance(); } | ||
| if (!digester.matches(oi.getDigest())) { throw OsGetDigestMismatch.instance(); } | ||
| String digest = oi.getDigest(); | ||
| if (digest == null || !digester.matches(digest)) { throw OsGetDigestMismatch.instance(); } |
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.
did I meantion nullability annotation that says getDigest can return null
| el.pullStatusWarning(null, null, null); | ||
| el.pullStatusError(null, null, null); | ||
| el.flowControlProcessed(null, null, null, null); | ||
| el.socketWriteTimeout(null); |
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.
coverage and tests from here down.
| public NatsUri nuri; | ||
| public boolean isGossiped; | ||
| public final NatsUri nuri; | ||
| public final boolean isGossiped; |
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.
these are set in the constructor and never changed. It's really an internal class anyway and I don't care if it breaks someone.
mtmk
left a comment
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.
LGTM
MauriceVanVeen
left a comment
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.
LGTM
| public Builder numReplicas(Integer numReplicas) { | ||
| this.numReplicas = numReplicas == null ? null : validateNumberOfReplicas(numReplicas); | ||
| this.numReplicas = numReplicas == null || numReplicas <= INTEGER_UNSET | ||
| ? null : validateNumberOfReplicas(numReplicas); |
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.
allowing unset to clear the value also
| this.mode = mode; | ||
| } | ||
|
|
||
| @NonNull |
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.
mode will never be null
| } | ||
|
|
||
| default void afterConstruct(Options options) {} | ||
| default void afterConstruct(@NonNull Options options) {} |
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.
will never be called with a null options
| } | ||
|
|
||
| private String lookupStreamSubject(String stream) throws IOException, JetStreamApiException { | ||
| StreamInfo si = _getStreamInfo(stream, null); |
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.
This function is not used
| // 1. If item is not found in the list being built, add to the list | ||
| for (NatsUri optionNuri : options.getNatsServerUris()) { | ||
| // If optionNuri is not found in the list being built, add to the list | ||
| // If optionNuri equivalent is found in the list and is secure, just use it |
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.
These comments explain the change.
| protected OutputStream out; | ||
|
|
||
| @Override | ||
| public void afterConstruct(Options options) { |
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.
same as interface default implementation, thanks IntelliJ
|
|
||
| @Override | ||
| public void afterConstruct(Options options) { | ||
| public void afterConstruct(@NonNull Options options) { |
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.
The interface marked this as non null since options will never be null
| StringListReader(String objectName) { | ||
| this(objectName, null); | ||
| } | ||
|
|
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.
internal and unused.
| private static NatsInetAddressProvider PROVIDER = new NatsInetAddressProvider() {}; | ||
|
|
||
| private NatsInetAddress() {} /* ensures cannot be constructed */ | ||
|
|
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.
makes coverage happy
| return null; | ||
| } | ||
| if (valueLength > 11) { | ||
| if (valueLength > 10) { |
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.
adding testing coverage discovered this bug
| public final class NatsSystemClock { | ||
| private static NatsSystemClockProvider PROVIDER = new NatsSystemClockProvider() {}; | ||
|
|
||
| private NatsSystemClock() {} /* ensures cannot be constructed */ |
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.
also make coverage happy
| } | ||
| } | ||
| void addServiceEndpoint(@NonNull ServiceEndpoint se) { | ||
| endpoints.add(new Endpoint( |
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.
this is internal and will never be called with a null ServiceEndpoint
No description provided.