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

Unit testability of Package #40

Closed
PascalVorwerkSaxion opened this issue Oct 31, 2023 · 14 comments
Closed

Unit testability of Package #40

PascalVorwerkSaxion opened this issue Oct 31, 2023 · 14 comments

Comments

@PascalVorwerkSaxion
Copy link

Describe the bug
I have implemented the deepl translator in my own package for own development. Now i am trying to unit test my own software, which means i want to essentially mock everything the translator performs. However, i can't simply Mock for example the Usage object, because it is a sealed class and it does not have an interface. I'm not sure if there is any other classes like this, but it would be nice to be able to mock these objects for tests.

Ofcourse, i can write my own layer above the translator object and Usage object, so that i can mock it.. but in my opinion this is a responsibility of the delivering library.

So.. sorry, not a bug, more like a feature request that would help every developer trying to integrate this package into their own software :)

If i've missed any approach on how to deal with the sealed classes without interface please let me know! I might have overlooked a easy way to fix the issue..

Screenshots or sample code
Heres an example of the code i would like to have in my tests:

image

@JanEbbing
Copy link
Member

Hi, I'll ask my colleague why we decided to seal these classes in the first place (my first reaction would be that anything we return should be an interface).

Until then, I see the following possibilities for you:

  • Bit of work: Write adapters/wrappers around our classes that allow you to mock them
  • Likely the best solution (+the next one): Refactor your code/Use dependency injection: If you want to test the case that e.g. you reached the character limit, there should be a function that takes a parameter if the character limit has been reached. Simply test that function instead of a bigger one that calls the translator as well.
  • To include the DeepL Library in your tests: Are you aware of our mock server? It is for integration tests, not unit tests (as it will still make local HTTP calls), but it allows you to easily test interactions with the DeepL libraries without having to make real requests over the wire.

I will get back to you on enabling mocking for internal library classes though.

@JanEbbing
Copy link
Member

1 more thing I just found - Translator offers an interface ITranslator and can thus be mocked. You could get around the internal constructor of Classes like Usage by using reflection. See this SO answer. (But this is obviously also rather ugly, I think having an adapter layer is more sensible)

@PascalVorwerkSaxion
Copy link
Author

PascalVorwerkSaxion commented Oct 31, 2023

Hi Jan!

Thanks for your quick response! I am aware about the your testing server and the server would be a possibility i am looking at for integration tests. However, my first step would be unit tests, to simply test my own logic.

I am already mocking the translator interface, but i also have to mock the responses that come out of this interface. For example, the only thing i am using of the Usage object is the AnyLimitReached boolean. So i would like to mock this behaviour like so:
image

In short, the Usage struct is a rather annoying object to work with from the outside, for example, simply creating an instance is also impossible because of the lack of a constructor that takes simple parameters. The model looks like it is only meant to be used for internal purposes, but it is exposed by a interface as a result.

I will probably take a look at how to implement a interface over the translator itself, so that i can mock the behaviour for sure. It is a rather ugly solution though.

@PascalVorwerkSaxion
Copy link
Author

PascalVorwerkSaxion commented Oct 31, 2023

I see that there are multiple models that use the sealed option, but also classes.

If the business logic is that Deepl does not want developers to inherit these classes, i would suggest making the implementations private, but a public sealed interface. This would allow for testing.. whilst enforcing the fact that the models cant be inherited.

Scratch above idea, sealed interfaces are also hard to mock.. Let me know if you guys would be open to making the models that you are exposing either public or an interface. Interfaces would be even easier to use and test :). I can also try to make a pull request.

@JanEbbing
Copy link
Member

JanEbbing commented Oct 31, 2023

For example, the only thing i am using of the Usage object is the AnyLimitReached boolean. So i would like to mock this behaviour like so:

Yes, if you want to unit test this way you have 3 options:

  • Use reflection to instantiate a Usage object (and other returned classes with an internal constructor), and return that in the ITranslator Mock. See the SO answer for how
  • In your integration of the library, build adapters/wrappers around our library that allow you to mock them. This is also better design as it makes it easier to integrate other translation solutions.
  • Use dependency injection and test the behaviour of your integration when e.g. the usage limit has been reached that way. See this reddit discussion

E: And yes, I agree we should not return Usage, but rather IUsage. Then Usage can be sealed and IUsage wont be. I will discuss this with my colleague tomorrow.

@PascalVorwerkSaxion
Copy link
Author

I have made an interface around my own service, however, each implementation of a specific translation solution provides such different approaches that a interface is... hard to realize? For example.. both Google and Azure don't give the possibilty to even look at usage amounts.

For now, i have simply removed the call to get the usage. But i would love it (also for the other models that are sealed) if they could become interfaces for testability! Looking forward to what you have discussed.

@PascalVorwerkSaxion
Copy link
Author

Hi! Any further news on how deepl is planning to address the testability of these classes that are returned by the client?

@JanEbbing
Copy link
Member

Hi, sorry for the delay. We are working on a change to allow you to mock a return value properly.
If you need this testability urgently, please see my previous comments about achieving this with an adapter or via reflection.

@PascalVorwerkSaxion
Copy link
Author

Hi Jan! Great to hear, i look forward to the changes. For now i've wrapped Deepl's models that i am using to ensure testability of my own software. No more urgent need, but would be nice to have :) Thanks for the support!

@redx177
Copy link

redx177 commented Nov 29, 2023

Another issue with testability are the exceptions: https://github.com/DeepLcom/deepl-dotnet/blob/main/DeepL/DeepLException.cs

All constructors are internal, so they can not be created to be returned by any mock.
It would also be enough to have a protected constructor so a new exception which extends DeepLException can be created for the purpose of testing.

@PascalVorwerkSaxion
Copy link
Author

^ Currently i am creating wrappers to simply wrap around the deepl exceptions for testing purposes.. alot of dumb boilerplate code for no reason, as the DeeplExceptions are essentially just wrappers themselves?

@redx177
Copy link

redx177 commented Feb 21, 2024

I used reflection. Pick your poison I guess....

var exceptionType = typeof(TooManyRequestsException);
_exception = (TooManyRequestsException)Activator.CreateInstance(exceptionType,
    BindingFlags.NonPublic | BindingFlags.Instance, null, new object[] { "exception message" }, null)!;

@redx177
Copy link

redx177 commented Apr 8, 2024

Exception have now public constructors. Thank you for this!
e88ce0c

@JanEbbing
Copy link
Member

Yes, thanks for the reminder :) We wanted to include arabic in the release, so this was a bit delayed.

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

3 participants