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

Optimizing unit tests #994

Merged
merged 2 commits into from
Jan 5, 2017

Conversation

Se7soz
Copy link
Contributor

@Se7soz Se7soz commented Jul 24, 2016

I'm trying to contribute to this repo and I have already used it at work to monitor our backend application .. so I've decided to start looking at the unit tests to understand more about how the whole lib work and found some optimizations that could be added to some of the unit tests

Copy link
Member

@arteam arteam left a comment

Choose a reason for hiding this comment

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

@Husseincoder, thanks for your desire to contribute! The change looks good, I've added one comment which need to be addressed.

input.close();
}
return builder.toString();
return new String(Files.readAllBytes(new File(dataDirectory, filename).toPath()));
Copy link
Member

Choose a reason for hiding this comment

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

Nice


public class DerivativeGaugeTest {
private final Gauge<String> gauge1 = new Gauge<String>() {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think there is a big improvement in replacing a simple gauge with a mock object. I would avoid mocking unless needed. Could you please revert this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@arteam arteam merged commit 36039b5 into dropwizard:3.2-development Jan 5, 2017
@arteam
Copy link
Member

arteam commented Jan 5, 2017

Thanks!

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.

2 participants