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.

Sign up for GitHub

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

Jump to bottom

Cleanup some test warnings and deprecated code #369

Merged
merged 6 commits into from
Mar 25, 2020
Merged

Cleanup some test warnings and deprecated code #369

merged 6 commits into from
Mar 25, 2020

Conversation

res0nance
Copy link
Contributor

Signed-off-by: Raihaan Shouhell raihaan.shouhell@autodesk.com

Cleanup some of the warnings

Cleanup some test warnings and deprecated code
5620c13 
Signed-off-by: Raihaan Shouhell <raihaan.shouhell@autodesk.com>
jeffret-b
jeffret-b approved these changes Feb 5, 2020
View reviewed changes
Copy link
Contributor

@jeffret-b jeffret-b left a comment

Choose a reason for hiding this comment

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

Thanks for the submission! It's nice to get things like this cleaned up.

I left a couple of comments for possible clarification or improvement.

src/test/java/org/jenkinsci/remoting/engine/JnlpProtocolHandlerTest.java Show resolved Hide resolved
src/test/java/org/jenkinsci/remoting/protocol/ProtocolStackImplTest.java Outdated Show resolved Hide resolved
@jeffret-b jeffret-b added the chore For changelog: A maintenance chore with no functional changes label Feb 5, 2020
MarkEWaite
MarkEWaite previously requested changes Feb 5, 2020
View reviewed changes
Copy link
Contributor

@MarkEWaite MarkEWaite left a comment

Choose a reason for hiding this comment

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

I believe there are at least two places where a try with resources should be used on an input channel in addition to the output channels.

src/test/java/hudson/remoting/DiagnosedStreamCorruptionExceptionTest.java Outdated Show resolved Hide resolved
src/test/java/hudson/remoting/TrafficAnalyzer.java Outdated Show resolved Hide resolved
src/test/java/hudson/remoting/TrafficAnalyzer.java Show resolved Hide resolved
src/test/java/org/jenkinsci/remoting/engine/JnlpProtocolHandlerTest.java
@@ -155,7 +155,7 @@ public void tearDown() {
}

@Theory
public void happyPath(Factory factory, boolean useNioHubServer, boolean useNioHubClient) throws Exception {
public void happyPath(Factory factory, boolean useNioHubServer, boolean useNioHubClient) throws Throwable {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand the motivation for this change. Can you explain further why the throw was broadened from Exception to Throwable? The stackoverflow article indicates that Throwable is broader than Exception, but I don't understand why a caller would need to know that this Theory may throw expections which are Throwable but are not Exceptions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My IDE was complaining about it, I don't mind reverting it

Copy link
Contributor

Choose a reason for hiding this comment

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

Reverting that would be better. I don't see that it needs to widened to Throwable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like you missed changing this one back.

Copy link
Contributor

Choose a reason for hiding this comment

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

Now I see why you needed to change this from an Exception to a Throwable. This change here changes the signature of this line here so that it now throws Throwable rather than Exception. Since it's test code, it should be okay changing the signature.

We've given you some bad advice about changing it back to Exception because we didn't understand how that other change ripples through.

This is why the build is failing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh well, It's all good.

The inheritance change is probably for the best so the class will advertise its exception and return type correctly

src/test/java/org/jenkinsci/remoting/protocol/ProtocolStackImplTest.java Outdated Show resolved Hide resolved
src/test/java/org/jenkinsci/remoting/protocol/impl/SSLEngineFilterLayerTest.java Show resolved Hide resolved
@jeffret-b
Copy link
Contributor

@res0nance, here are three things I think would be worth doing in this changeset.

  1. Change the "throws Throwable" back to "throws Exception". (I think we're agreed on that one.)
  2. Since you are adding the try-with-resources, include the enhancements Mark proposed.
  3. (Optional) Try to fix the close(null) problem instead of adding the TODO comments.

res0nance and others added 3 commits February 7, 2020 09:12
@MarkEWaite MarkEWaite removed their request for review February 8, 2020 21:01
jeffret-b
jeffret-b approved these changes Feb 11, 2020
View reviewed changes
Copy link
Contributor

@jeffret-b jeffret-b left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks again for the submission.

@jeffret-b jeffret-b merged commit df84c5d into jenkinsci:master Mar 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeffret-b jeffret-b jeffret-b approved these changes

@MarkEWaite MarkEWaite MarkEWaite left review comments

Assignees
No one assigned
Labels
chore For changelog: A maintenance chore with no functional changes ready-to-merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@res0nance @jeffret-b @MarkEWaite