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

Split historyEngine.go into small files #5972

Conversation

taylanisikdemir
Copy link
Member

@taylanisikdemir taylanisikdemir commented May 6, 2024

What changed?
historyEngine.go is a giant file with ~4k lines which makes it hard to maintain/review and add tests for.
Splitting to small files (mostly one file per public method) to make it more testable.

There are 3 separate test suites with some overlap of what they are testing

  • engineSuite
  • engine2Suite
  • engine3Suite

In follow up PRs we are going to add test files for each file and split (get rid of) those suites as well.

Why?
Make it more readable and testable

How did you test it?
unit tests

Copy link

codecov bot commented May 6, 2024

Codecov Report

Attention: Patch coverage is 40.20530% with 1398 lines in your changes are missing coverage. Please review.

Project coverage is 63.97%. Comparing base (a843a9e) to head (d115906).
Report is 1 commits behind head on master.

❗ Current head d115906 differs from pull request most recent head f57c17b. Consider uploading reports for the commit f57c17b to get more accurate results

Additional details and impacted files
Files Coverage Δ
.../engine/engineimpl/record_decision_task_started.go 100.00% <100.00%> (ø)
...gine/engineimpl/respond_decision_task_completed.go 100.00% <100.00%> (ø)
...y/engine/engineimpl/remove_signal_mutable_state.go 85.71% <85.71%> (ø)
.../engine/engineimpl/respond_decision_task_failed.go 0.00% <0.00%> (ø)
...tory/engine/engineimpl/cross_cluster_operations.go 0.00% <0.00%> (ø)
service/history/engine/engineimpl/reset_queues.go 0.00% <0.00%> (ø)
...history/engine/engineimpl/reset_sticky_tasklist.go 0.00% <0.00%> (ø)
...ine/engineimpl/record_child_execution_completed.go 77.41% <77.41%> (ø)
...gine/engineimpl/respond_activity_task_completed.go 74.19% <74.19%> (ø)
...rvice/history/engine/engineimpl/describe_queues.go 0.00% <0.00%> (ø)
... and 20 more

... and 6 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 4cb4443...f57c17b. Read the comment docs.

@taylanisikdemir taylanisikdemir force-pushed the taylan/split_history_engine branch from d115906 to f57c17b Compare May 7, 2024 18:28
@taylanisikdemir taylanisikdemir enabled auto-merge (squash) May 7, 2024 18:30
@taylanisikdemir taylanisikdemir merged commit 2ac145f into cadence-workflow:master May 7, 2024
18 checks passed
@taylanisikdemir taylanisikdemir deleted the taylan/split_history_engine branch May 7, 2024 19:46
abhishekj720 pushed a commit to abhishekj720/cadence that referenced this pull request May 7, 2024
abhishekj720 pushed a commit to abhishekj720/cadence that referenced this pull request May 7, 2024
agautam478 pushed a commit to agautam478/cadence that referenced this pull request May 8, 2024
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.

3 participants