jkube icon indicating copy to clipboard operation
jkube copied to clipboard

Replace mocking in tests with builders in unit tests

Open ShivangMishra opened this issue 2 years ago • 12 comments

Component

None

Task description

Description

image Currently, in BaseGeneratorTest, JavaExecGeneratorTest, WebAppGeneratorTest, etc. we use mockito to mock the generator which we want to test. We can replace the mocks by building the generator and the java project using the corresponding JavaProject and GeneratorContext builders.

Expected Behavior

This is an example of how the tests should be written - image

Here's an example with the same properties image

Acceptance Criteria

  • [ ] Mocks should be replaced with builders wherever possible

ShivangMishra avatar Aug 02 '23 06:08 ShivangMishra

@rohanKanojia does this issue look okay? I'll create a PR for this one after GSoC.

ShivangMishra avatar Aug 02 '23 06:08 ShivangMishra

Looks okay to me. Could you also list the test classes that would require changes in order to fix this issue?

I'll create a PR for this one after GSoC.

I think someone else can also pick it up if we explain the issue clearly.

rohanKanojia avatar Aug 02 '23 07:08 rohanKanojia

@rohanKanojia lots of test classes use mock. I'll make a google sheet and will add it in the description. Would that be okay?

@ShantKhatri if you're interested, please go through this issue and make a list... You can start by making similar changes in the generators. Make a draft PR once this much is done.

ShivangMishra avatar Aug 06 '23 19:08 ShivangMishra

@ShivangMishra : Yeah, sounds good. Just make sure permissions are open for anyone.

rohanKanojia avatar Aug 07 '23 03:08 rohanKanojia

I am not sure to understand the value, it makes the test "bigger" from what I see

sunix avatar Aug 10 '23 09:08 sunix

I am not sure to understand the value, it makes the test "bigger" from what I see

Instead of using mocks, we should test on real objects. As far as I understand, we should use mocks only when we cannot directly test on real objects(maybe because the implementation is not complete). This will prevent complications like we got that Null Pointer Exception earlier.

Also, it won't make the tests bigger. In this example, setup() in the screenshot at the bottom is bigger because I have used some extra properties. I'll update it with a better example.

ShivangMishra avatar Aug 10 '23 09:08 ShivangMishra

@rohanKanojia @ShivangMishra I have created list of files which includes mock testing. Here is the google sheet: https://docs.google.com/spreadsheets/d/14bcELvvAVEx5bI30GNgktjqVe6zrx562pRNjjVeI7aA/edit?usp=sharing Could you please guide me more on this? so I can start work on it.

ShantKhatri avatar Aug 13 '23 10:08 ShantKhatri

Hi @ShantKhatri, Are you working on this?

manusa avatar Sep 05 '23 09:09 manusa

Hi @ShantKhatri, Are you working on this?

I was waiting for reply from @rohanKanojia and @ShivangMishra that from above sheet in which files mocking should we have to replace with generator tests?

ShantKhatri avatar Sep 05 '23 10:09 ShantKhatri

@rohanKanojia @manusa Do I need to create a separate issue for each file on which I'm working, or can I create a PR for this (https://github.com/eclipse/jkube/issues/2316) issue only? In that case, there would be multiple PRs linked with a single issue.

ShantKhatri avatar Feb 04 '24 10:02 ShantKhatri

I'd start just with one of the files so that you get feedback on your approach.

manusa avatar Feb 05 '24 06:02 manusa

I'd start just with one of the files so that you get feedback on your approach.

@manusa Could you please guide me with approach?

ShantKhatri avatar Feb 07 '24 13:02 ShantKhatri