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 tests for stateManagerCtx #558

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

KentHsu
Copy link

@KentHsu KentHsu commented May 1, 2024

Description

This PR add tests for stateManagerCtx
mockgen a mock_client file so stateManagerCtx can run tests with this mock client
Added tests for methods in stateManagerCtx and fixed a bug in setWithTTL method

Issue reference

Please reference the issue this PR will close: #446

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Created/updated tests
  • Extended the documentation

@KentHsu KentHsu requested review from a team as code owners May 1, 2024 13:43
Copy link

codecov bot commented May 1, 2024

Codecov Report

Attention: Patch coverage is 6.20690% with 544 lines in your changes missing coverage. Please review.

Project coverage is 58.44%. Comparing base (27248ba) to head (4b0276c).
Report is 24 commits behind head on main.

Files Patch % Lines
actor/mock_client/mock_client.go 4.28% 514 Missing ⚠️
actor/mock/mock_server.go 11.53% 23 Missing ⚠️
actor/mock/mock_manager.go 50.00% 5 Missing ⚠️
actor/mock/mock_container.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #558      +/-   ##
==========================================
+ Coverage   58.04%   58.44%   +0.40%     
==========================================
  Files          55       57       +2     
  Lines        3568     3973     +405     
==========================================
+ Hits         2071     2322     +251     
- Misses       1375     1518     +143     
- Partials      122      133      +11     

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

@mikeee mikeee added this to the v1.11 milestone May 2, 2024
Copy link
Member

@mikeee mikeee left a comment

Choose a reason for hiding this comment

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

Great stuff, just a few initial comments from my first look.

  • Would you be able to include tests for the deprecated statemanager methods until they're removed?
  • Should the makefile be updated to include a new target that can generate mockfiles using the same flags and which mockgen version/fork you're using e.g. https://github.com/uber-go/mock and a corresponding version

A side note is that I believe we should exclude generated mocks from codecov scans - possibly in a separate PR.

@KentHsu
Copy link
Author

KentHsu commented May 3, 2024

Thank you for the feedback. I have updated this PR with

  • Added tests for stateManager methods. These tests are duplicates of stateManagerCtx tests.
  • Added a new target in Makefile to generate mocks and updated mocks. I'm using golang mock because this is how those mock were generated. mock_factory_impl.go is not created by mockgen so it's not in the Makefile. mock_server.go is updated but I didn't put it in the Makefile because it was manually edited.
  • Put mocks in .codecov.yaml ignore. Let me know if this is okay. I'll revert it if it's inappropriate.

@mikeee
Copy link
Member

mikeee commented May 5, 2024

Just had another skim-read, looking great so far. I haven't forgotten and will pick the review up tomorrow when I get a moment 👌

@KentHsu
Copy link
Author

KentHsu commented May 5, 2024

Sure. Please take your time 👍
BTW, I realize that I do need another PR to update .codecov.yaml... I'll submit another PR to update it when this one is ready.

@mikeee
Copy link
Member

mikeee commented May 6, 2024

Is there any reason against using Uber's maintained fork to generate mock interfaces?

@KentHsu
Copy link
Author

KentHsu commented May 6, 2024

I tried both and I didn't see anything blocking us from using Uber's fork.
If we want to switch to Uber's fork, I'll have to update go.mod and other test files using these mocks since Uber's fork doesn't work with those original mocks. Is that okay?

@KentHsu
Copy link
Author

KentHsu commented May 6, 2024

I went ahead to make some commits to use Uber's gomock fork and revert the change in .codecov.yaml. Please let me know if there's any concern. Thanks.

@KentHsu
Copy link
Author

KentHsu commented May 21, 2024

Hi @mikeee, would you like me to create another PR to exclude the mock files so that this PR won't be block by code coverage? Any suggestions on the code change in this PR?

@yaron2
Copy link
Member

yaron2 commented Jul 22, 2024

@KentHsu please resolve the conflicts

@KentHsu
Copy link
Author

KentHsu commented Jul 23, 2024

Hi @yaron2, @mikeee , I just resolved the conflicts and updated mocks. Please let me know if there's anything I can do to get this PR merged. Thank you.

@mikeee mikeee modified the milestones: v1.11, v1.12 Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Needs Owner
Development

Successfully merging this pull request may close these issues.

test: we should add tests for the stateManagerCtx
3 participants