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

Restore ability to mock internal classes #326

Closed
pamcevoy opened this issue Sep 3, 2016 · 20 comments
Closed

Restore ability to mock internal classes #326

pamcevoy opened this issue Sep 3, 2016 · 20 comments
Assignees
Labels

Comments

@pamcevoy
Copy link

pamcevoy commented Sep 3, 2016

As of JMockit 1.27 mocking of classes from the same codebase produces an error saying it is not allowed and mocking private methods produces an error. I see you closed issue 324 so this is an enhancement.

Can you remove that check or put in a way to disable it - maybe with SuppressWarnings? I find it useful to mock out methods that setup FileWriters and replace them with mock methods that setup StringWriters so no files are written during test runs. I could probably use Deencapsulation to modify it or make the method protected and then use a subclass, but I don't always have direct access to the class - e.g. I am testing some class that uses a helper object that has a FileWriter.

Thanks.

@rliesenfeld
Copy link
Member

@SuppressWarnings is not available at runtime (it has "source" retention), so it wouldn't be an option.
Anyway, the idea is to eventually turn this warning into an exception.

I will wait a few months to see if more users want the ability to have mockups for internal classes.
Ideally, the @mocked API should be used for classes inside the codebase under test, with MockUp's being used for external libraries only.

@pamcevoy
Copy link
Author

pamcevoy commented Sep 4, 2016

Thanks for keeping an open mind - and for explaining SuppressWarnings. I really appreciate the tool.

@hamid-nazari
Copy link

Inline with this topic I have a question:
Since mocking of internal classes is considered not a good practice, and while mocking of private and protected methods are not available through @moked API anymore, then how would you suggest one can tackle the problem of writing tests for class C which depends on class D (from the same code base) that involves mocking a protected method of class D?

@rliesenfeld
Copy link
Member

Mocking of protected methods continues to be supported by @mocked.

On Mon, Sep 5, 2016 at 10:59 AM, hamid-nazari notifications@github.com
wrote:

Inline with this topic I have a question:
Since mocking of internal classes is considered not a good practice,
and while mocking of private and protected methods are not available
through @moked https://github.com/Moked API anymore, then how would you
suggest one can tackle the problem of writing tests for class C which
depends on class D (from the same code base) that involves mocking a protected
method
of class D?


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#326 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAMOurJEmRzcMjvoKzlPWcpeNuNnTPybks5qnCAmgaJpZM4J0ZBf
.

@nebel
Copy link

nebel commented Sep 7, 2016

Hi. Regarding the decision to warn when using the MockUp API for internal classes, I understand your reasoning. However, I hope you consider that the MockUp API has great value even when @mocked and the Expectations API aren't being used.

To give an example, in my company we are doing a lot of API testing using JUnit with a custom test framework that loads API requests and responses from config files and performs a direct call to the Spring methods to process that request. Each request config file specifies not just the payload but other parameters, including which mocks to use for that request. So based on the individual test case config, these mocks (which are based on MockUp) are dynamically loaded and instantiated for the duration of that request. So while we make heavy use of MockUp in these particular tests, we don't actually use the Expectations API at all in these tests. (Of course, in other kinds of tests, we make use of both.)

I think this flexibility of JMockit is one of the great things about it. I understand that you don't want to encourage bad behavior for the most common use cases, but I think that keeping this kind of powerful flexibility is valuable.

@WtfJoke
Copy link

WtfJoke commented Sep 12, 2016

We recently ran in the same issue (Invalid mock-up for internal class...), while testing the upgrade of jmockit in our codebase.

What is considered as internal class? Every class, which is not part of an external library?
We make heavy use of MockUps (2000+) in our Tests and all of them are used that way.

@rliesenfeld
Copy link
Member

Yes, classes not from external libraries are "internal".

Note the "Invalid mock-up for internal class" is just a warning, not an
exception that gets thrown.

I will probably add a system property to allow for internal mock-ups, to be
used in existing test suites.
But it's recommended to avoid such mock-ups going forward.

On Mon, Sep 12, 2016 at 10:10 AM, Manuel notifications@github.com wrote:

We recently ran in the same issue (Invalid mock-up for internal class...),
while testing the upgrade of jmockit in our codebase.

What is considered as internal class? Every class, which is not part of an
external library?
We have tons of MockUps (2000+) used in our Tests and all of them are used
that way.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#326 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAMOurYKfLIFvB71rSIOsyTkmHWYcFNpks5qpU9fgaJpZM4J0ZBf
.

@rliesenfeld
Copy link
Member

Version 1.28 will add a system property "allowed-internal-mockups", which can list the names of internal mockup classes. For these, the warning won't be shown.

@charlesritchea
Copy link

How do you use this property? I set the property and added the names of the classes to it, but I'm getting the Warning and an IllegalArgumentException as well.

@dgeissl
Copy link

dgeissl commented Oct 20, 2016

I don't want to rage here, just a little thinking about the direction jmockit is going.
I see that you want to promote your vision of the right way of testing to the users of your library.
I am still surprised how powerful jmockit is and learning new and better ways with every test i write.

My problem is that the real world is not just about good code. One is often facing a ugly old codebase where you need to fix that one special bug and then leave the rest behind.
You neither have the time nor the intention to fix all those design and structural issues that where made in the past, but you want to get a basic feeling that your bugfix might work in production too.
So you mock as much as possible, isolate the problem, fix it and leave the test there so no one else will break it again.

Mocking final Singletons even with SingletonHolders was no problem. Mocking out pretty big private methods as they add nothing to the code i wanted to test, but just increase the number of
expectations i needed to setUp to get to the point, easy. Kill static initializers cause they make some crazy jndi Lookup that breaks the test on the client, can be done.

But the last releases seem to kill one feature after another to force the users to write tests that apply to your or Martin Fowlers (academic?) vision of mock tests.
Breaking changes with nearly every minor relase (semantic versioning anyone?) and not knowing what will be cut out with the upcoming ones is realy not what i expected when i made my decision for jmockit.
All i can read from comments is "you are not doing it right, so either stay with old version and get cut off from fixes and features or rewrite your tests".
Please reconsider this practice and give people some more backward compatibility or at least a long migration path.

For me personally i was switching from 1.23 to 1.27 and had to change ca. 40% of my very small jmockit testbase. I dont want to know what happens if there where hundereds of them (just dopping them and leaving untested code behind? not realy).
Currently the warning "Invalid mock-up for internal class" leaves me in doubt if this will get broken with any upcoming release. So how am i supposed to mock that final Singleton in the future (@capturing refuses to mock final classes, new MockUp in the same code base maybe broken...)?

Thank you for reading

@rliesenfeld
Copy link
Member

rliesenfeld commented Nov 15, 2016

So far, absolutely no concrete evidence has been shown that mockups for internal classes are useful in good tests. From the looks of it, no evidence will be shown. So, most likely the warning will become an exception in version 1.30 or 1.31.

@nebel
Copy link

nebel commented Nov 16, 2016

Wouldn't all these people wanting to use them qualify them as useful? Many arguments have been given, and I think it's a little silly to ask for "concrete evidence" when testing philosophy is obviously a matter of taste.

I don't think sticking so closely to some ideological notion of "good testing" is really that productive, if doing so harms testing in practice. In practice, lots of people end up doing useful testing even without having the privilege of being able to write ideal tests in an ideal code base. I know that personally I don't always have the time, freedom or ability to dig in and rework existing code bases when writing tests, for many good and valid reasons.

In an ideal world we'd all love to be writing only good code and testing it with good tests. In practice, people often end up writing good tests for bad code. This is where JMockit shines, and I really don't understand why these features are being removed. If your code base is easy to test, great. And making a code base easier to test in a noble endeavor. But why punish those people stuck with difficult and hard to test code bases? Why punish the person who wants to get their tests up and working quickly so they know their code works, then dig in and refactor it all later? Why break thousands of working test cases because a person wants to upgrade for a bug fix?

It's fine to have a testing philosophy, but why not give the users of JMockit the freedom to decide on their own philosophy too?

@numeralEasy
Copy link

Why don't you add a mode?
Something like:
setMode(Mode.UNTESTABLE_CODE); //everything is mockable, good ol' jmockit.
setMode(Mode.C); //closer to martin fowler, features removed
setMode(Mode.B) //closer to martin fowler, features removed
setMode(Mode.A) //closer to martin fowler and robert .c martin, features removed
setMode(Mode.MATRIN_FOWLER_AND_ROBERT_C_MARTIN) // worlds most clean code, features removed

@rliesenfeld rliesenfeld self-assigned this Nov 16, 2016
@rliesenfeld
Copy link
Member

@nebel @numeralEasy You know what, I think you're right!

I was just thinking about this whole issue of "preventing misuse/abuse of the mocking/faking APIs" today... It sure would be nice to get rid of all that validation code, making the JMockit codebase smaller, simpler, faster, and avoiding the occasional bug or unexpected problem arising from said code. Also, in the end people will always find ways to write bad tests. (I recently had the opportunity to examine one such test suite of 1500+ tests which used Mockito in a terrible way - it could just as well have used JMockit or any other mocking library.)

So, instead of the validations, I am going to just add some advice in the documentation. This should also free more time for me to work in the coverage tool, which still needs major improvements.

@catkeeper1
Copy link

Hi @rliesenfeld ,

I just upgrade jmockit to 1.29 and found all my test case that mock private method failed. After I read the release note, I found:

  1. the support for private method mocking in Expecations API has been dropped in 1.23.
  2. In 1.26, invocations, minInvocations, and maxInvocations for Mock annotation is removed.
  3. In 1.27, validation is added against MockUp with internal class.

After I read your comments in some issues items, I understand you want to do so because you want people test base on the behavior but not implementation detail. For this point, I am 200% agree with you.However, please do not assume that, for every java projects, a "unit being tested" means one java class. I focused on enterprise application development(use Spring mainly). In my project, most of the times, the business logic is not that simple, For example, we have one Service (I mean the service objects in Spring) object to handle order creation. The order is complicated. We need to do validation 1, validation 2, validatioan 3. Then, save data come from UI into different tables. Generate audit log base on data from UI. Call workflow engine to submit order. Now, our practice is, we create a public method createOrder(). createOrder() call private methods validate1(), validate2(), validate3(), saveDate() and generateAuditLog(). If I use jmockit 1.29 to test this program, I should not mock any private methods in internal classes. Which means, when I want to test createOrder(), I only have 2 options:

  1. Mock all dependences for createOrder(), validate1(), validate2(), validate3(), saveDate() and generateAuditLog().; That means, I mean need to do mocking for more than 10 Dao and external service object which is very complicated.
  2. Change validate1(), validate2(), validate3(), saveDate() and generateAuditLog() to public. However, this is not a good design because we do not expect these methods are called by other classes.
  3. I change the design, move validate1(), validate2(), validate3(), saveDate() and generateAuditLog() to another classes. The issues for this approach is: a)This approach is similar to approach 2 because we also need to changes all them to public but this is not good because we do not hope other service can call them. b) Programmers will not be happy with this approach. They prefer the original design because in original design the whole jave file(for that service object that create order) is about 400 lines. Each method is about 50 - 60 lines(the max no of line < 100) so that it is easy to read and understand. It is not needed to create another file or class for those private method. Furthermore, all those methods are highly related to order creation, if we put all of them in one file, developer can read all the names of them at one time in the outlet view of IDE(for example, the eclipse outlet view can show all method of one class). This is easier for them to read source and trace issue(anyway, the programmers in my team prefer do so). c) Client will not agree with this approach(This is the most important point). Before we use jmockit, the system is already there. Now, we just want to use jmockit to create some UTs to cover some high risk parts. If I told them I need to change the program structure just because a mocking tool ask us to do so and client need to redo a lot of acceptation test (I know the logic is not changed but that is their policy. Every modification need acceptation test. I cannot change that) they will kill me.

So, the testing approach I want is:

  1. Every method is a unit tha1t will be tested. I will test them base on their behavior
  2. First, we need to test the behavior of createOrder(). It should invoke all other private method in an order we expected. In this test case, we just mock the private methods.
  3. Then, we test those private methods one by one. In these test case, we mock the DAO or external service for that private method only
  4. Finally, we get a few test cases to cover that service objects and each test case is short and simple.

Conclusions:

  1. I understands you hope your users can write good test. However, if they want to do bad things you cannot stop them no matter how many limitation you add. People will still find way to do so. So, the key point is education. I suggest you add more documents and example to highlight what is good test.
  2. Software development is very complicated. When you build something, please do not assume how your users will use it because you cannot predict what situation your users will face.
  3. Please remove those validation and warning message about internal class mock up.
  4. Please recover the private method mocking in Expectations API. Sometimes, I just want to test whether a private methods have been called 3 times as I expect, I need to build a program to count. However, in Expectations API, I just need to write "times = 3". That is much simpler.

@eoghanoh1
Copy link

I moved from 1.24 to 1.29 and I am getting the same InvalidArgumentException and warning.

This issue has been closed, how was it resolved and what version of JMockit is it resolved in?

Just to clarify why I am mocking an internal class. The class that is being mocked is the entry point for network communication with the application that is under test. In the test suite, data received on the socket is mocked and injected into the application by mocking this class.

@charlesritchea
Copy link

charlesritchea commented Dec 14, 2016 via email

@charlesritchea
Copy link

charlesritchea commented Feb 3, 2017 via email

@dellgreen
Copy link

cool thanks just noticed 1.30 available in netbeans and this does indeed fix things for me

@m-kuklinski
Copy link

I second wanting private mocking like this.

'Good' tests may not need to internals, but you often have to write tests for code you didn't write where you actually need to validate something specific got hit, or need to mock a specific internal to make the test remotely reasonable. Not all codebases are sane. There's no good reason to deny users the ability to do this. "Not a good practice" is subjective and defendant upon factors outside of your control, and it is best to leave it to the programmers discretion. Make a PrivateExpectations or something, so code analysis tools can mark it as a smell.

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

No branches or pull requests