IEP-946 Test coverage for ExecutableFinder

[sigmaaa]

Description

PR summary:

• migrating to JUnit5 existing test for IDFVersionReader
• test coverage for ExecutableFinder and the following refactoring
• Made ExecutableFinder abstract so we can create fake objects for further testing and be less coupled to this util class
• Added SystemWrapper interface which makes ExecutableFinder less coupled to System.getenv functionality. It allows us to avoid using PowerMock
• Added test coverage for SystemWrapper as well.

Fixes # (IEP-946)

Type of change

Please delete options that are not relevant.

• New feature (non-breaking change which adds functionality)

How has this been tested?

• test install tools functionality. We can still find the git, and python paths from the system
• run junit tests

Test Configuration:

• ESP-IDF Version:
• OS (Windows,Linux and macOS):

Dependent components impacted by this PR:

• Install Tools,
• Install Tools Wizard

Checklist

• PR Self Reviewed
• Applied Code formatting
• Added Documentation
• Added Unit Test
• Verified on all platforms - Windows,Linux and macOS

[sigmaaa]

@kolipakakondal, @alirana01
FYI: I left some comments to explain some design choices

[sigmaaa]

probably we can move this class to unittest package and leave the test package for other kinds of tests

[kolipakakondal]

LGTM

[alirana01]

I have left a comment that we can probably look into and if needed I can explain on call my idea related to design. The other changes you mentioned I think are valid and can be done just try to take a look at the big test class.

[sigmaaa]

Removed an unused method from the ExecutableFinder interface and cleaned up unreachable code

[alirana01]

Hi @sigmaaa thank you for explaining I just went over your PR again I am a bit confused about the interfaces you are using and I am not able to get the idea why we need them. About the test cases and this I think we can discuss this on a call it would be better

[sigmaaa]

Hi @sigmaaa thank you for explaining I just went over your PR again I am a bit confused about the interfaces you are using and I am not able to get the idea why we need them. About the test cases and this I think we can discuss this on a call it would be better

Hi @alirana01. I prefer using interfaces for several reasons, but in this case mostly because of these ones:

1. They leave space for future refactoring/extensibility since we can rely on abstraction and easily replace components in test scenarios.
2. Interfaces make it easier to stub and mock dependencies in your tests. By programming to interfaces, you can create mock implementations or stub implementations of those interfaces, which can be used to simulate specific behaviors and control the responses of the dependencies during testing.

[alirana01]

Hi @alirana01. I prefer using interfaces for several reasons.................

Hi @sigmaaa thanks for the response, I think you are pretty clear on the design and it makes sense to me now in many places. Only thing I would suggest is that my last discussion over stuff when added for future references with @kolipakakondal led to the outcome that we are supposed to keep YAGNI or KISS in mind. If you think it satisfies those things and is viable for not so distant PRs its good to go.

Happy Testing :)

[alirana01]

@sigmaaa about interfaces I can agree that it maynot be a standard convention but it increases the readability by a lot.
Anyway these are some good references I found them helpful
https://stackoverflow.com/a/542097/1722792
https://stackoverflow.com/a/542012/1722792
https://stackoverflow.com/questions/541912/interface-naming-in-java

[alirana01]

Just take a look at the comments mentioned and see if you can incorporate @kolipakakondal should be the final person to finalize design side stuff

[sigmaaa]

Hi @alirana01,

thank you for your feedback. I'm always trying to keep in mind KISS and YAGNI principles, that's the reason why during this refactoring I removed one method from public API and also removed the boolean parameter :) I do plan on using interfaces to mimic ExecutableFinders in future tests, which I will add soon.

As for the “I” prefix, for me personally, it hurts readability, because we always have to rely on abstraction as much as possible, so treat interfaces like any other type. I think "I" is a standard convention for C# where it is not clear that we are extending an implementation class or interface.
People mention it actually in the mentioned discussions.

Regardless, I'm grateful for our discussion. Taking the time to contemplate matters repeatedly is always beneficial and provides ample mental stimulation :D

[kolipakakondal]

Hi @sigmaaa,

Thanks for the PR! I understand that you introduced the ExecutableFinder and SystemWrapper interfaces for abstraction and flexibility in the design. However, we don't currently have any different implementations for these interfaces.

In general, it's best to only introduce interfaces when you have multiple classes that will implement them, each with a different implementation. This allows us to decouple the different parts of our code and make it more flexible.

If you've designed these interfaces with future extensibility in mind, and you see some use cases coming up where we'll need different implementations, then it's probably fine to leave them as-is. Otherwise, PR looks good to me.

[kolipakakondal]

I see builds are getting failed, we can fix and merge this PR. @sigmaaa

[sigmaaa]

Hi @sigmaaa,

Thanks for the PR! I understand that you introduced the ExecutableFinder and SystemWrapper interfaces for abstraction and flexibility in the design. However, we don't currently have any different implementations for these interfaces.

Hi @kolipakakondal, thanks for the review. We already using different implementations of SystemWrapper in the tests. I'm planning to use different implementations of ExecutableFinder in future tests for IDFUtil class (but first need to make a dependency injection there). It's possible to mock a class for this purpose instead of a mock interface, but I've heard that some Mockito frameworks don't support mocking classes, so it's better to not rely on it and use an interface for abstraction.

I see builds are getting failed, we can fix and merge this PR. @sigmaaa

fixed in the last commit