gojuno / minimock

Powerful mock generation tool for Go programming language
MIT License
632 stars 38 forks source link

feat: added option -t to support generating package name with suffix "_test" #69

Closed zcolleen closed 9 months ago

zcolleen commented 1 year ago

Hi! Sometimes, when you write your tests in "_test" package, you need to generate mocks with the same package name, but there is no option to do so. I ve added option -t that will generate package name with "_test" suffix. It will also add _test to filename if it is not added yet, because you cant generate other files as there will be package conflicts "Multiple packages in the directory"

zcolleen commented 1 year ago

@hexdigest hi! Is there any option to review this request? It would help me a lot!

hexdigest commented 1 year ago

Hey, @zcolleen !

Few comments:

  1. Minimock is already applying _mock_test.go suffix to the generated files by default. In case when -t flag is specified and the file doesn't have "_test.go" suffix ( which means that file name was manually specified) minimock has to return an error rather than forcing file name. Because the user specified conflicting command line arguments.
  2. In general I think it would be better to allow users to specify any package name rather than handling this particular case, i.e. -p "mypackage_test".
  3. If you're willing to implement (2) than please consider that minimock allows you to specify multiple inputs and outputs:
    -n Mock
        comma-separated mock names,
        by default the generated mock names append Mock to the given interface name
    -o string
        comma-separated destination file names or packages to put the generated mocks in,
        by default the generated mock is placed in the source package directory

    So this new flag should support this too.

zcolleen commented 1 year ago

@hexdigest thank you for your feedback. I ve implemented -p flag as you said, supporting multiple values, added some test cases. I ve also removed all validation functionality because if user can specify any package name, its his responsibility to check if name is correct. What do you think about all this?

defaulterrr commented 1 year ago

@hexdigest thank you for your feedback. I ve implemented -p flag as you said, supporting multiple values, added some test cases. I ve also removed all validation functionality because if user can specify any package name, its his responsibility to check if name is correct. What do you think about all this?

I'm not quite sure about removing the validation as it provides a baseline check. At least notifying user about incorrect package name would be nice

zcolleen commented 1 year ago

@hexdigest hi! sorry for asking again but is there any opportunity to merge this pr please?

hexdigest commented 9 months ago

Hey @zcolleen

Excuse me for the delayed response, I'm pretty busy these days. Can you please update your branch?

zcolleen commented 9 months ago

Hey @hexdigest I did the update on my branch

By the way, did you intentionally deleted snapshot tests? Why?

hexdigest commented 9 months ago

Hey @hexdigest I did the update on my branch

By the way, did you intentionally deleted snapshot tests? Why?

Hi, yes. The reason is that I don't see very much value in these tests. We already generate mocks that we actually test and by generating these mocks you can already see the diff between old and new mock template.

The only thing we check by comparing the generated code to the snapshot is that it's identical. But obviously when you make a change to the mock template you want to make the resulting code different, so having these snapshots just adds more overhead to maintenance.