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

Fixing mocks for interfaces where method returns function with variadic argument. #719

Merged
merged 3 commits into from
Jan 12, 2024

Conversation

nicovak
Copy link
Contributor

@nicovak nicovak commented Oct 9, 2023

Description

Adding a Variadic fixtures to handle signatures with that case

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Version of Golang used when building/testing:

  • 1.20

How Has This Been Tested?

  • Adding fixtures with case and assert output directly in mocks folder, I may need help or guidance for this one.

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

@nicovak nicovak force-pushed the nicovak/fix-707 branch 2 times, most recently from 650440b to 4fd9909 Compare October 9, 2023 09:59
@codecov
Copy link

codecov bot commented Oct 10, 2023

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (2502f52) 75.26793% compared to head (b6625de) 75.16447%.

Files Patch % Lines
pkg/generator.go 38.46154% 6 Missing and 2 partials ⚠️
Additional details and impacted files
@@                 Coverage Diff                 @@
##              master        #719         +/-   ##
===================================================
- Coverage   75.26793%   75.16447%   -0.10347%     
===================================================
  Files             10          10                 
  Lines           2426        2432          +6     
===================================================
+ Hits            1826        1828          +2     
- Misses           461         464          +3     
- Partials         139         140          +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

Thank you for sending in a PR. A few questions in the comments.

@nicovak
Copy link
Contributor Author

nicovak commented Oct 13, 2023

Thank you for sending in a PR. A few questions in the comments.

You are welcome, I answered them and just pushed the case for #431 which works with my original code

@egeucak
Copy link

egeucak commented Dec 11, 2023

are there plans to merge this? i am also facing an issue with variadic arguments

@LandonTClipp
Copy link
Contributor

@egeucak I believe we are still waiting on @nicovak to address my last comment. It's very close to being done but we just need someone to address that thread.

@nicovak
Copy link
Contributor Author

nicovak commented Dec 12, 2023

I will try to fix It this week

@nicovak
Copy link
Contributor Author

nicovak commented Dec 12, 2023

@LandonTClipp @egeucak I rebased and pushed with the fix we were talking about.
It should be fine now, I added the case about the array in the VariadicExtra Interface

@LandonTClipp LandonTClipp changed the title fix: issue #707, issue #431 Fixing mocks for interfaces where method returns function with variadic argument. Dec 20, 2023
@LandonTClipp
Copy link
Contributor

Great, thank you @nicovak ! Could you add a test next to pkg/fixtures/variadic.go that uses the generated mock object to ensure we can specify the return argument we expect? You might need to place the mock implementation next to pkg/fixtures/variadic.go (instead of in the mocks/ subdirectory) due to circular import issues.

@mapkon
Copy link

mapkon commented Jan 10, 2024

When will this be merged?

@LandonTClipp
Copy link
Contributor

We need tests for this particular case. I believe my last comment is still pending. How big of a blocker is this for you? I can probably finish it out myself.

@mapkon
Copy link

mapkon commented Jan 11, 2024

We need tests for this particular case. I believe my last comment is still pending. How big of a blocker is this for you? I can probably finish it out myself.

We have a pending PR, that we can't merge unless this is fixed.

@LandonTClipp
Copy link
Contributor

We have a pending PR, that we can't merge unless this is fixed.

Okay I will just complete it tomorrow.

@LandonTClipp
Copy link
Contributor

I submitted the remaining fixes I wanted and assured this works correctly. Will merge in a few moments!

@LandonTClipp LandonTClipp merged commit 4854efd into vektra:master Jan 12, 2024
2 of 4 checks passed
@nicovak
Copy link
Contributor Author

nicovak commented Jan 15, 2024

Thank you @LandonTClipp , I didn't have time to work on It these days and I think I would not have been able to implement the tests quicky.

@LandonTClipp
Copy link
Contributor

LandonTClipp commented Jan 15, 2024

Thank you @LandonTClipp , I didn't have time to work on It these days and I think I would not have been able to implement the tests quicky.

No worries at all, we're all busy nowadays. Teamwork makes the dream work and all 😉

Thanks for the contribs!

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.

Wrong code generation when using ellipsis
4 participants