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

WIP make generated struct name configurable #75

Closed relnod closed 5 years ago

relnod commented 5 years ago

This adds the --name flag to the generate command. When specified the generated mock struct name will be set accordingly. The default is the interface name prefixed with "Mock", which is how it is currently generated. Therefore it should be backwards compatible.

This is still wip, although it is working, when tested manually. I should propably add some tests? If so, where would be the best place? In generall I'm still a little confused, by your tests... Is the general change ok?

petergtz commented 5 years ago

Hey @relnod,

Thanks for your PR.

I think the general change makes sense, yes. Sorry, the test structure is probably a bit confusing, because I want to test the DSL with all different "backends" that are supported, i.e. I have 3 different ways to parse the code to get an AST out of code (it's all located in the modelgen directory). So scripts/run_tests.sh first generates the mock source based on one of the 3 backends and then runs the DSL tests.

But long story short, I think for your change it makes sense to add a test in pegomock/main_test.go. In this one you specify the command that you also run manually and then simply verify that the source code contains the named mock as specified on the command line, or that it uses the default name, if not specified on the command line. So probably 2 additional tests should be fine.

One more comment about the new flag: maybe it should be named --mock-name. I know it's longer, but I think it would make it more explicit what this is about.

Thanks, Peter

relnod commented 5 years ago

Hi @petergtz, Thanks for you feedback! I've added two tests in pegomock/main_test.go and renamed the flag to be --mock-name

petergtz commented 5 years ago

Looks good. Can't merge it right now, but will probably do it later today.

Paul Schiffers notifications@github.com schrieb am Mi., 13. Feb. 2019, 13:59:

Hi @petergtz https://github.com/petergtz, Thanks for you feedback! I've added two tests in pegomock/main_test.go and renamed the flag to be --mock-name

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/petergtz/pegomock/pull/75#issuecomment-463190223, or mute the thread https://github.com/notifications/unsubscribe-auth/ADc2YR4rM3LZfb0oqE2BkRbpsG9tlxKdks5vNAxNgaJpZM4a0bC4 .

petergtz commented 5 years ago

Hey @relnod,

I had a more detailed look now and it seems it's breaking the use case where you specify .go files as parameters to the generate command:

Context("with args mydisplay.go", func() {
    FIt(`generates a file mock_mydisplay_test.go that contains "package pegomocktest_test"`, func() {
        main.Run(cmd("pegomock generate mydisplay.go"), os.Stdout, os.Stdin, app, done)

        Expect(joinPath(packageDir, "mock_mydisplay_test.go")).To(SatisfyAll(
            BeAnExistingFile(),
            BeAFileContainingSubString("package pegomocktest_test"),
            BeAFileContainingSubString("MockMyDisplay"),
        ))
    })
})

If you check the main_test.go file for this test case you won't find it exactly. I've just added the BeAFileContainingSubString("MockMyDisplay") which your change will break. The reason is that you assume you have the interface name from the command. But this is not the case when specifying .go files directly. It might be a bit confusing, but I think it becomes clearer when you run the tests again with that additional BeAFileContainingSubString("MockMyDisplay").

The possibility of specifying .go files instead of package+interface is something that I want to get rid of eventually, but not at this point yet. It will also require a major version bump etc.

I think the way to solve it, is to construct the mock name not in main.go, but rather in mockgen.go in the generateMockFor method. So instead of doing the if construct with realMockNameOut you just pass down mockNameOut. Then you can do a similar logic in generateMockFor.

relnod commented 5 years ago

Hey @petergtz , sorry for the delay. I changed the behaviour as you mentioned. The mock name is now determined in mockgen.go. I also changed the test you mentioned to check for "MockMyDisplay".

petergtz commented 5 years ago

Nice work! Thanks a lot for your contribution, @relnod. Will create a new release with this change in a few minutes.