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

Print the fully qualified class names if the simple class names are the same #2311

Closed
kluever opened this issue Jun 1, 2021 · 10 comments
Closed

Comments

@kluever
Copy link
Contributor

kluever commented Jun 1, 2021

I have 2 overloads of a method:

  • setDeadline(java.time.Duration)
  • setDeadline(org.joda.time.Duration)

There's a mocking failure in a test (calling the wrong overload), but the error message isn't super helpful:

1) myMethod(com.google.frobber.MyClassTest)
Argument(s) are different! Wanted:
myClass.setDeadline(
    (Duration) PT1S
);
-> at com.google.frobber.MyClassTest.myMethod(MyClassTest.java:287)
Actual invocation has different arguments:
myClass.setDeadline(
    (Duration) PT1S
);

The output attempts to include the classnames to help disambiguate, but in this case, the simple class names are identical (Duration). We should be able to detect this, and print the FQCNs if the simple names are equal.

Alternatively, we could always print the FQCNs:

return "(" + wanted.getClass().getSimpleName() + ") " + describe(wanted);

(We already do something similar in Truth.)

@TimvdLippe
Copy link
Contributor

This is probably a relatively simple change. For anybody who is looking to contribute to Mockito, this is a good starter bug!

@saurabh7248
Copy link
Contributor

I would love to try to fix this.

@TimvdLippe
Copy link
Contributor

@saurabh7248 Awesome. Looking forward to the PR!

@chaehwanli
Copy link

Sorry, I got some misunderstanding. I revert my PR. :)

@TimvdLippe
Copy link
Contributor

@chaehwanli Looking at https://github.com/chaehwanli/mockito/commit/2175bfb43e75d80976d75ab0338aaf751cdee9eb I think that would be the correct fix for this. Do you mind sending that as a PR?

@chaehwanli
Copy link

@TimvdLippe
Oh.. thank you for your review~ I will send again at night in KST. I'm at office. :)

saurabh7248 pushed a commit to saurabh7248/mockito that referenced this issue Jun 9, 2021
Prints fully qualified class name when the simple names of arguments match.
@saurabh7248
Copy link
Contributor

saurabh7248 commented Jun 9, 2021

@chaehwanli's changes would have printed the full name even when the classes would have different simple names.
I have added a list of indexes in PrintSettings which would do this only for arguments having same simple names.

@TimvdLippe
Copy link
Contributor

@saurabh7248 Ah good catch. Thank you so much both of you to work on this and iterating on a solution 😄 That approach SGTM. I think there are some small improvements we can make in terms of code structure, but I like that approach as well.

If you could open a PR, then we start discussing the implementation there and iterate on that separately from this issue thread. Thank you for your work thus far!

@saurabh7248
Copy link
Contributor

Have opened a pull request here.

saurabh7248 pushed a commit to saurabh7248/mockito that referenced this issue Jun 13, 2021
Made changes to Equals.toStringWithType by reusing same method to achieve both cases by sending a boolean as an input, based on which either simple name or fully qualified name will be used for describing the type.

Also in the method ArgumentMatchingTool.getNotMatchingArgsWithSameName return Set<String> which return the simple names of classes having more than one classes with different FQCN.
saurabh7248 pushed a commit to saurabh7248/mockito that referenced this issue Jun 26, 2021
Fixed the style issues, by running gradlew :spotlessApply

Also needed to handle the case where argument is null, which gave null pointer while getting class name from the object.
@TimvdLippe
Copy link
Contributor

Fixed in 1ad8235

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants