Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add constructor for mocks #406

Merged
merged 6 commits into from Apr 18, 2022

Conversation

grongor
Copy link
Contributor

@grongor grongor commented Sep 13, 2021

These changes will simplify the tests by not being required to assert mocks expectations every time. This will also eliminate situations where your tests are not failing even though they should, only to find out that you forget the defer statement.

Example
Before:

factory := &mocks.Factory{}
defer factory.AssertExpectations()

something := &mocks.Something{}
defer something.AssertExpectations()

something.On("GetTime").Return("tooLate")

factory.On("Create").Return(something)

testThings(factory)

Now:

factory := mocks.NewFactory()
something := mocks.NewSomething()

something.On("GetTime").Return("tooLate")

factory.On("Create").Return(something)

testThings(factory)

edit: this already got merged: These changes are based on a branch where I did some refactoring, you can see it here #399. If you don't intend to merge the refactoring PR, I can rebase this branch onto the master.

@LandonTClipp
Copy link
Contributor

@grongor can you please rebase onto master and fix the conflicts? Thanks!

@grongor
Copy link
Contributor Author

grongor commented Sep 16, 2021

@LandonTClipp done :)

pkg/generator.go Outdated Show resolved Hide resolved
@LandonTClipp
Copy link
Contributor

@grongor I tried fixing a merge conflict but messed up a closing bracket, could you fix that? I will merge once the tests pass.

@grongor
Copy link
Contributor Author

grongor commented Mar 31, 2022

@LandonTClipp Done :). Also please note that the Expecter PR did more changes than it should (refactoring, new way to check generated code, ...), specifically expectedBytes, err := ioutil.ReadFile(getFixturePath("mocks", "expecter.go")). I'm not saying that it is bad...just that this is the first place in the codebase that it is done this way, so I had to look around for some time until I managed to find a way to fix the errors after rebasing. I also reverted part of the Expecter PR because it caused panics during tests (it's in a separate commit, please check).

@LandonTClipp
Copy link
Contributor

Thanks @grongor, can you elaborate on what you mean by "new way to check generated code"?

Copy link
Contributor

@LandonTClipp LandonTClipp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR looks good, I only have one comment about the export/unexported name logic but that doesn't hold up this PR. If you want extra credit you can make the helper function :)

pkg/generator.go Show resolved Hide resolved
Makefile Show resolved Hide resolved
pkg/generator_test.go Show resolved Hide resolved
@LandonTClipp
Copy link
Contributor

Because we're dealing with some issues regarding go1.18 builds I'm going to pause this PR to minimize changes, but once that settles down this is good to go.

@grongor
Copy link
Contributor Author

grongor commented Apr 3, 2022

Can you elaborate on what you mean by "new way to check generated code"?

this file: pkg/fixtures/mocks/expecter.go

The rest of the generated mock files live in mocks directory. The generator test contains expected code directly in the test file pkg/generator_test.go, and there is only one test using an "external" file pkg/fixtures/mocks/expecter.go. Maybe there is a good reason for it - I don't know. I just wanted to point out that it kind of does things differently than the rest of the tests.

Because we're dealing with some issues...

No problem, I'm glad that it's finally moving :) Thanks!

@codecov-commenter
Copy link

codecov-commenter commented Apr 5, 2022

Codecov Report

Merging #406 (eddf049) into master (4703d1a) will increase coverage by 0.31%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master     #406      +/-   ##
==========================================
+ Coverage   69.80%   70.12%   +0.31%     
==========================================
  Files           7        7              
  Lines        1262     1292      +30     
==========================================
+ Hits          881      906      +25     
- Misses        333      338       +5     
  Partials       48       48              
Impacted Files Coverage Δ
pkg/fixtures/mocks/expecter.go 84.00% <0.00%> (-3.50%) ⬇️
pkg/generator.go 90.81% <100.00%> (+0.39%) ⬆️

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 4703d1a...eddf049. Read the comment docs.

@LandonTClipp LandonTClipp merged commit 32dd223 into vektra:master Apr 18, 2022
@LandonTClipp
Copy link
Contributor

@grongor merged and deployed. One additional matter, could you document this in the README as a new feature that is available? I'm sure others will benefit from it.

@grongor
Copy link
Contributor Author

grongor commented Apr 19, 2022

@LandonTClipp thank you! I'll look into updating the README soon, as well as the helper function you mentioned (#406 (comment)).

@grongor
Copy link
Contributor Author

grongor commented Apr 19, 2022

Readme update: #450

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants