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

[Work In Progress] [POOL-238] Improve test generics #374

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

rajucomp
Copy link

Thanks for your contribution to Apache Commons! Your help is appreciated!

Before you push a pull request, review this list:

  • Read the contribution guidelines for this project.
  • Run a successful build using the default Maven goal with mvn; that's mvn on the command line by itself.
  • Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied. This may not always be possible but is a best-practice.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Each commit in the pull request should have a meaningful subject line and body. Note that commits might be squashed by a maintainer on merge.

@rajucomp
Copy link
Author

@garydgregory I have created a draft pull request for you to review at https://github.com/apache/commons-pool/pull/374 . There is some finishing touches still required but I want to know if this is something you would consider worth reviewing, Let me know your thoughts on this.

Copy link
Member

@garydgregory garydgregory left a comment

Choose a reason for hiding this comment

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

Hello @rajucomp
I am sorry but this PR is too way big and messy to review. If this is about generics, then ONLY make it about generics please. The PR adds Mockito which is not needed to test generics IMO. There are also cosmetic changes it seems. A PR should be focused. I'd rather see effort in fixing bugs than picking up an 11 year old Jira ticket.

public String create() {
return String.valueOf(counter++);
public T create() {
return (T)String.valueOf(counter++);
Copy link
Member

Choose a reason for hiding this comment

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

This does not make sense to me. What am I missing? If I declare a SimpleFactory<Integer, RuntimeException>, what happens?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah. That will be cleaned up.

@rajucomp
Copy link
Author

Hello @rajucomp I am sorry but this PR is too way big and messy to review. If this is about generics, then ONLY make it about generics please. The PR adds Mockito which is not needed to test generics IMO. There are also cosmetic changes it seems. A PR should be focused. I'd rather see effort in fixing bugs than picking up an 11 year old Jira ticket.

The way the tests are currently being tested is not efficient. We use a class called MethodCall to verify function calls which does not seem the right way. Changing the test to use generics will require changes in the MethodCall class as well. I could try to shorten the PR to include only minimal generic changes.

Later, I can raise a PR that includes Mockito changes. Does that make sense ?

@garydgregory
Copy link
Member

@rajucomp
I'm not going to spend time reviewing changes to tests that are not required or needed for a bug fix.
If you want to help this component, look at bug fixes.

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