petergtz / pegomock

Pegomock is a powerful, yet simple mocking framework for the Go programming language
Apache License 2.0
253 stars 28 forks source link

Add the ability to run pegomock to remove mocks and matchers #72

Closed jpopadak closed 5 years ago

jpopadak commented 5 years ago

Many of our projects are becoming very large, as a result, we have a ton of mocks being created. Due to us not committing autogenerated files, switching between branches becomes a pain. Because some branches will have files that do not exist in other branches.

I propose a change that will run through all generated mocks and matchers, and remove the ones that do not get generated. Or, something that will just remove all mocks and matchers regardless if they are to be generated.

I dont quite know how this will work, but it would be very useful, more than just writing our own custom script.

petergtz commented 5 years ago

Hey @jpopadak, I think it sounds like a valid use case.

So could you elaborate on the details? I.e. did you intend to call this within a package and then it would remove everything generated recursively within and below this package?

If so, the question is how to identify generated files. I guess we could simply search for // Code generated by pegomock. DO NOT EDIT., because this is available in every generated file.

Is this what you had in mind?

jpopadak commented 5 years ago

I honestly do not know the best way to do this without having some sort of state file that shows all the mocks created or the way you suggested (but adding that the filename must be matchers/ or mock_<name>_test.go).

But overall, yes, this is what I was thinking about for this issue.

petergtz commented 5 years ago

Hi @jpopadak, I made a prototype for your request. It lives on the remove-generated branch. Feel free to try it out and let me know if that fits your use case. Once you're happy, I'll merge it into master :-).

jpopadak commented 5 years ago

Three suggestions:

(1) We should only read files that have .go extensions. I get the following errors when doing recursive. Could not open file /Users/james.popadak/go/src/github.com/<name>/<name>/vendor/github.com/grpc-ecosystem/go-grpc-middleware/auth/README.md. Error: &os.PathError{Op:"open", Path:"/Users/james.popadak/go/src/github.com/<name>/<name>/vendor/github.com/grpc-ecosystem/go-grpc-middleware/auth/README.md", Err:0x2}

(2) We should delete the matchers folder if it is empty (returning the repo to the original state).

(3) Update the readme with information on how to run this for others. I see you still need to update the command documentation too.

Besides this, it worked like a dream with our repo. It has about 160 files total in many different directories. It works with no input directory recursion and with a specified directory.

petergtz commented 5 years ago

@jpopadak (1) Yea, definitely fair point. I can't see a scenario where files would not have a .go extension. Will add this. That said, I'm surprised it couldn't read the README your referencing. Any idea?

(2) That would make things a bit cleaner, yes. That said, I think for switching branches, it shouldn't matter, right? Because git doesn't care about empty folders. I'll think about how to do this, because I don't want to randomly delete empty matchers folders that weren't used for pegomock matchers before. Seems like this isn't a blocker in general though, unless you can explain in which cases this causes trouble with switching branches.

(3) Yea, will definitely update the doc. I justwanted to get your feedback on this from you first.

jpopadak commented 5 years ago

(1) It looks like the file is a symlink that links to another file that dep did not pull down (in another repository). (2) Correct, git does not care about empty folders. But if I am switching branches and refactoring code, one branch might have matchers in that directory and another branch might not due to those folders and files not existing on that branch. Thus, by leaving those empty directories, they have no context in what is happening for that branch. To continue this, you should (I dont know how the code is structured) be able to see that you had just deleted matchers in that folder, and then, if the folder is empty, delete the folder. (3) Sweet!

petergtz commented 5 years ago

@jpopadak

Thus, by leaving those empty directories, they have no context in what is happening for that branch.

I'm not sure I understand this sentence. Could you elaborate? Specifically, who are "they"?

jpopadak commented 5 years ago

They refers to the empty directories in that branch.

  1. So, I checkout a branch b1
  2. Set it up to create matchers for an interface for a given test, test t1, in that branch. It creates a matchers directory next to t1.
  3. One then calls pegomock remove to remove all mocks and matchers.
  4. Switch to branch b2
  5. Branch b2 does not have that given interface nor that test. The way git will operate will be to keep those empty folders.

In theory, the matchers folder can be 4 directories deep with all the directories being empty if there is no code files in those directories anymore (due to the branch switching).

petergtz commented 5 years ago

@jpopadak OK. So this is really about not having empty directories lying around that provide no context to the developer, not about git. I have an implementation almost ready.

I have some concern though: I wouldn't want to arbitrarily delete empty directories upwards as long as they're empty. Even if they're empty, that would seem pretty invasive. I can see that we want to delete the matchers dir, but only that. Leave all (potentially empty) directories that originally contained matchers alone. Do you agree?

jpopadak commented 5 years ago

I agree, only the matchers directory is what I was talking about because it is not generated code / directories (sorry about the confusion).

petergtz commented 5 years ago

@jpopadak OK, final version (hopefully :-) ) before it goes to master. Could you please try this latest version? Still on the remove-generated branch. This should remove the matchers folder after removing all generated matcher files in it.

jpopadak commented 5 years ago

Looks and works great, however 1) The command line help text has not been updated and 2) the readme has not been updated either. But the code works fine for my local instance.

petergtz commented 5 years ago

Yep, will update both and then it'll go into Master.

jpopadak notifications@github.com schrieb am Mi., 5. Dez. 2018, 18:15:

Looks and works great, however 1) The command line help text has not been updated and 2) the readme has not been updated either. But the code works fine for my local instance.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/petergtz/pegomock/issues/72#issuecomment-444566785, or mute the thread https://github.com/notifications/unsubscribe-auth/ADc2YbJJ4mwtsyXg-WGw-HuilBVNLu08ks5u1_9FgaJpZM4XVyIp .

petergtz commented 5 years ago

@jpopadak It's done. If you're happy with this, feel free to close it.

jpopadak commented 5 years ago

Looks good to me. 👍 I was going to approve the PR associated with it, didn't realize you never created it. 😆