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

Trivial code cleanup for tests #114

Merged
merged 19 commits into from
Aug 19, 2019
Merged

Conversation

basil
Copy link
Member

@basil basil commented Jul 30, 2019

A series of trivial code cleanup changes for the tests in this repository. Most of these changes remove usages of deprecated functionality or otherwise fix IDE warnings. I kept each commit separate. Each commit message should be self-explanatory, but if anything is unclear I am happy to provide further explanation. Also, feel free to revert any specific cleanup commits that are unwanted.

@basil basil closed this Jul 30, 2019
@basil basil reopened this Jul 30, 2019
Copy link
Member

@dwnusbaum dwnusbaum left a comment

Choose a reason for hiding this comment

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

Looks ok to me, thanks!

@@ -126,52 +114,6 @@
});
}

/** Ephemeral and deleted when it disconnects */
static class EphemeralDumbAgent extends Slave implements EphemeralNode {
Copy link
Member

Choose a reason for hiding this comment

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

I was confused about this earlier as well, looks like it was added in #47, but then the test using it was deleted in #48, but the class stayed around unused.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed. Fun fact: #48 was reverting #47 because it broke a production use case for me!

// Ensure deleted
f1.delete();
// Ensure file does not exist
assertFalse(Files.exists(f1));
Copy link
Member

Choose a reason for hiding this comment

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

Did the old behavior matter if this plugin's test were being run in the same workspace more than once? I guess you could use Files.deleteIfExists(f1) to preserve the old behavior, but I'm not sure what the original concern was.

Copy link
Member Author

Choose a reason for hiding this comment

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

I too was having a hard time understanding the original behavior. Your explanation is as good as any, so we might as well preserve the existing functionality. I switched to Files.deleteIfExists and updated the comment to reflect your explanation.

Copy link
Member

@dwnusbaum dwnusbaum 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, I'll merge once the CI build passes (ping me if it completes and it looks like I forgot about it 😄).

@dwnusbaum
Copy link
Member

java.nio.channels.ClosedChannelException

The Windows agent was broken, rebuilding.

@dwnusbaum dwnusbaum closed this Aug 13, 2019
@dwnusbaum dwnusbaum reopened this Aug 13, 2019
@dwnusbaum
Copy link
Member

Another java.nio.channels.ClosedChannelException on the Windows agent, rebuilding.

@dwnusbaum dwnusbaum closed this Aug 13, 2019
@dwnusbaum dwnusbaum reopened this Aug 13, 2019
@basil basil closed this Aug 19, 2019
@basil basil reopened this Aug 19, 2019
@basil
Copy link
Member Author

basil commented Aug 19, 2019

Looks good, I'll merge once the CI build passes (ping me if it completes and it looks like I forgot about it smile).

I think this should be good to go now.

@dwnusbaum dwnusbaum merged commit 42bbddf into jenkinsci:master Aug 19, 2019
@basil basil deleted the improvements branch August 19, 2019 17:21
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