Move generated ExpecterTest to mocks directory #454

Gevrai · 2022-04-20T00:12:54Z

Description

Fixes the location of the --expecter tests mocks, as requested in #396 (comment)

Type of change

Test files moved to correct location, new behaviour change for users.

Version of Golang used when building/testing:

How Has This Been Tested?

Tested with go test ./... on full project

Checklist

My code follows the style guidelines of this project
I have performed a self-review of my code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes

codecov-commenter · 2022-04-20T00:14:29Z

Codecov Report

Merging #454 (265dd0d) into master (aad3571) will decrease coverage by 24.94%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           master     #454       +/-   ##
===========================================
- Coverage   70.76%   45.81%   -24.95%     
===========================================
  Files           7       40       +33     
  Lines        1293     1997      +704     
===========================================
  Hits          915      915               
- Misses        325     1029      +704     
  Partials       53       53

Impacted Files	Coverage Δ
mocks/pkg/fixtures/ExpecterTest.go	`84.00% <100.00%> (ø)`
mocks/pkg/fixtures/requester_unexported.go	`0.00% <0.00%> (ø)`
mocks/pkg/fixtures/RequesterSlice.go	`0.00% <0.00%> (ø)`
mocks/pkg/fixtures/KeyManager.go	`0.00% <0.00%> (ø)`
mocks/pkg/fixtures/AsyncProducer.go	`0.00% <0.00%> (ø)`
mocks/pkg/fixtures/Example.go	`0.00% <0.00%> (ø)`
mocks/pkg/fixtures/MapFunc.go	`0.00% <0.00%> (ø)`
mocks/pkg/fixtures/RequesterArray.go	`0.00% <0.00%> (ø)`
mocks/pkg/fixtures/ConsulLock.go	`0.00% <0.00%> (ø)`
mocks/pkg/fixtures/MapToInterface.go	`0.00% <0.00%> (ø)`
... and 25 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aad3571...265dd0d. Read the comment docs.

Gevrai · 2022-04-20T01:28:47Z

@LandonTClipp As far as I can see the command make mocks doesn't generate all the right ones in the mocks file, is that expected? I didn't add this one for the expecter because it seemed outdated, but I might be wrong.

grongor · 2022-04-20T11:07:49Z

mocks/pkg/fixtures/ExpecterTest.go

@@ -1,18 +1,26 @@
-// Code generated by mockery. DO NOT EDIT.
+// Code generated by mockery v1.0.0. DO NOT EDIT.


This looks like you used the wrong version to generate the mocks...strange.

Agreed, could you verify what mockery you're using to build the mocks? These version strings should be disabled so this is telling me that your mockery probably wasn't reading the config properly. (maybe we should re-introduce the version numbers?)

I hadn't used the Makefile, fixed now

grongor · 2022-04-20T11:43:20Z

@Gevrai I think the mocks are generated correctly (but I might be wrong, please post details if you are sure). The issue here is that you combine the generated mocks and the fixtures in an unusual way for this repo IMO. All generator tests have their expected output in the generator test itself. I think the best way forward is to just update the expecter test to behave the same as other tests, and I'll look into it later and try to move these fixed strings to separate files.

The problem with what you did (I think) is that you say that you expect the generated code to be equal to the mock file, but the mock file itself is generated by the generator, so this test doesn't really validate anything. I might be wrong though...

Gevrai · 2022-04-20T14:49:29Z

@Gevrai I think the mocks are generated correctly (but I might be wrong, please post details if you are sure). The issue here is that you combine the generated mocks and the fixtures in an unusual way for this repo IMO. All generator tests have their expected output in the generator test itself.

Oh, my bad I didn't think those were autogenerated anymore, when doing make clean mocks locally it doesn't really seem to be working as expected, some are modified/deleted. I half expected the script to not be used 😅

I saw that there were some other tests reading from expected file (TestGeneratorVariadicArgs and TestGeneratorVariadicArgsAsOneArg) and went that route because it made the most sense to me. I don't exactly remember why I had put it in a new mocks directory initially (it's been more than 6 months now..), maybe because I saw the other files were generated, I'm not sure.

I think the best way forward is to just update the expecter test to behave the same as other tests, and I'll look into it later and try to move these fixed strings to separate files.

I would kind of want to keep the unit tests for the autogenerated expecter (file now at mocks/pkg/fixtures/ExpecterTest_test.go). I can't add those tests if I have them inlined, unless maybe next to the autogenerated version?

I would be strongly in favor having the fixed string in separate files, maybe even some kind of setup were we can have a full test defined in a file as

// Interface definition to test
// Generator Config
// Expected output

The problem with what you did (I think) is that you say that you expect the generated code to be equal to the mock file, but the mock file itself is generated by the generator, so this test doesn't really validate anything. I might be wrong though...

You are right, for the future at least. I didn't run regenerate the directory on my side so for now so it is indeed validating that the output is valid, but that would be overwritten as soon as the directory is regenerated...

LandonTClipp · 2022-04-21T15:29:14Z

pkg/generator_test.go

@@ -240,7 +240,7 @@ func NewRequester(t testing.TB) *Requester {
 }

 func (s *GeneratorSuite) TestGeneratorExpecterComplete() {
-	expectedBytes, err := ioutil.ReadFile(getFixturePath("mocks", "expecter.go"))
+	expectedBytes, err := ioutil.ReadFile(getMocksPath("ExpecterTest.go"))


The thing I agree with @grongor here is that it's odd to be asserting the output of mockery with the output of mockery (you see what I mean?). You are generating ExepcterTest.go in the mocks package, then you generate it again in this test and assert that it's the same. It's not necessarily wrong to do it this way, but none of the other tests do it this way (see TestGeneratorFunction below).

Although, I can see an argument to be made that because the Expecter interface is so verbose in the generated code, that cluttering the tests with expected code would be too much. But I think that for this case, we really should have the expected code as a raw string in the test.

I think we should stick with how the tests are done for now (strings directly in the test), but definitely look for improvements later on. As you mentioned, asserting mockery with mockeries output isn't correct, but we definitely can improve the readability of the tests, maybe even the way @Gevrai described. But it definitely is out of the scope of this, and it's a bigger decision that should be done correctly (so that we don't face it again in a year, hopefully :D ).

I do really like the idea of keeping unit tests for the generated mocks (mocks/pkg/fixtures/ExpecterTest_test.go), that can probably stay. I think we should have more of these unit tests on the generated mocks as another layer.

@Gevrai let me know if you could make the change (expected output as a string literal in the test). We can get it merged once that's settled. Thanks!

Hey sorry! Things got in the way a bit, I'll have it done this evening.

Just to make sure

Keep test in mocks/pkg/fixtures/ExpecterTest_test.go

Inline generated expecter check instead of reading from file

No worries thanks again for the contributions. Yeah those two points should be it! (and the merge conflicts that just popped up)

Done, should be good now.

FYI I've changed the name of the interface (ExpecterTest -> Expecter) for coherence with others, and added the expecter to be generated by Makefile so that the unit tests have something to run on!

Gevrai · 2022-05-24T18:38:15Z

branch rebased

LandonTClipp · 2022-05-24T19:20:49Z

Thanks @Gevrai you read my mind.

grongor reviewed Apr 20, 2022

View reviewed changes

LandonTClipp requested changes Apr 21, 2022

View reviewed changes

Gevrai force-pushed the gejo-move-expecter-test branch 2 times, most recently from fe03e28 to f9cad3c Compare April 26, 2022 03:36

Move generated ExpecterTest to mocks directory

cde38d9

Gevrai force-pushed the gejo-move-expecter-test branch from f9cad3c to cde38d9 Compare May 24, 2022 18:32

LandonTClipp approved these changes May 24, 2022

View reviewed changes

LandonTClipp merged commit 9dba1fd into vektra:master May 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move generated ExpecterTest to mocks directory #454

Move generated ExpecterTest to mocks directory #454

Gevrai commented Apr 20, 2022 •

edited

Loading

codecov-commenter commented Apr 20, 2022 •

edited

Loading

Gevrai commented Apr 20, 2022

grongor Apr 20, 2022

LandonTClipp Apr 20, 2022

Gevrai Apr 26, 2022

grongor commented Apr 20, 2022

Gevrai commented Apr 20, 2022

LandonTClipp Apr 21, 2022 •

edited

Loading

grongor Apr 21, 2022

LandonTClipp Apr 21, 2022

LandonTClipp Apr 25, 2022

Gevrai Apr 25, 2022

LandonTClipp Apr 25, 2022 •

edited

Loading

Gevrai Apr 26, 2022

Gevrai commented May 24, 2022

LandonTClipp commented May 24, 2022

		@@ -1,18 +1,26 @@
		// Code generated by mockery. DO NOT EDIT.
		// Code generated by mockery v1.0.0. DO NOT EDIT.

Move generated ExpecterTest to mocks directory #454

Move generated ExpecterTest to mocks directory #454

Conversation

Gevrai commented Apr 20, 2022 • edited Loading

Description

Type of change

Version of Golang used when building/testing:

How Has This Been Tested?

Checklist

codecov-commenter commented Apr 20, 2022 • edited Loading

Codecov Report

Gevrai commented Apr 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grongor commented Apr 20, 2022

Gevrai commented Apr 20, 2022

LandonTClipp Apr 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LandonTClipp Apr 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gevrai commented May 24, 2022

LandonTClipp commented May 24, 2022

Gevrai commented Apr 20, 2022 •

edited

Loading

codecov-commenter commented Apr 20, 2022 •

edited

Loading

LandonTClipp Apr 21, 2022 •

edited

Loading

LandonTClipp Apr 25, 2022 •

edited

Loading