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

Unit tests for activity task handler #1378

Conversation

3vilhamster
Copy link
Contributor

@3vilhamster 3vilhamster commented Nov 1, 2024

Detailed Description
To improve test coverage, I've moved existing components to separate files and added unit tests for the activity handler.

Impact Analysis

  • Backward Compatibility: Full
  • Forward Compatibility: Full

Testing Plan

  • Unit Tests: Unit tests and existing integration tests test
  • Persistence Tests: unrelated
  • Integration Tests: This could be verified with existing unit/integration tests and on canary deployment.
  • Compatibility Tests: Unrelated

Rollout Plan

  • What is the rollout plan? Simple rollout starting with canary
  • Does the order of deployment matter? No
  • Is it safe to rollback? Does the order of rollback matter? Yes
  • Is there a kill switch to mitigate the impact immediately? Rollback

Copy link

codecov bot commented Nov 1, 2024

Codecov Report

Attention: Patch coverage is 97.38562% with 4 lines in your changes missing coverage. Please review.

Project coverage is 77.92%. Comparing base (a71b70f) to head (cf98af2).
Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
internal/activity_task_handler.go 96.77% 3 Missing and 1 partial ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
internal/activity.go 89.71% <ø> (ø)
internal/auto_heartbeater.go 100.00% <100.00%> (ø)
internal/internal_task_handlers.go 81.61% <ø> (+0.17%) ⬆️
internal/activity_task_handler.go 96.77% <96.77%> (ø)

... and 2 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

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


require.True(t, util.AwaitWaitGroup(wg, time.Second))
})
t.Run("heartbeat fail", func(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

why not use table test to have less redundancy? this approach is hard to maintain especially while the function under test changes. requires changing N test bodies instead of 1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if I can save much with table tests.
I think a setup will have many customizations and be hard to read.

internal/activity_task_handler_test.go Outdated Show resolved Hide resolved
@3vilhamster 3vilhamster force-pushed the refactor-activity-task-handlers branch from 29c3fbe to ec10286 Compare November 1, 2024 20:17
@3vilhamster 3vilhamster merged commit 5b97f10 into cadence-workflow:master Nov 1, 2024
14 checks passed
@3vilhamster 3vilhamster deleted the refactor-activity-task-handlers branch November 1, 2024 21:07
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.

2 participants