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

History engine start/stop unit tests #5985

Conversation

taylanisikdemir
Copy link
Member

What changed?
It is now possible to write small scoped unit tests for history engine after it got splitted into small files in #5972.
This PR adds a basic Start/Stop test.
Changes:

  • Generated some mocks that were missing
  • Introduced a QueueProcessorFactory to easily mock queue processors. Previously they were created inside constructor. This will also be handy when we try different queue implementations.

Why?
Improve coverage and make code testable.

How did you test it?
Unit tests

type EngineForTest struct {
Engine engine.Engine
Cleanup func()
// Add mocks or other fields here
Copy link
Member Author

Choose a reason for hiding this comment

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

Planning to attach mock objects created inline inside NewEngineForTest function here so different unit test scenarios can adjust the behavior/expectations

Copy link

codecov bot commented May 8, 2024

Codecov Report

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

Project coverage is 65.06%. Comparing base (73e6dbc) to head (a640d04).

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

Additional details and impacted files
Files Coverage Δ
...ervice/history/engine/engineimpl/history_engine.go 83.67% <100.00%> (+78.36%) ⬆️
service/history/replication/task_fetcher.go 26.71% <ø> (ø)
service/history/handler/handler.go 44.37% <0.00%> (-0.04%) ⬇️
service/history/queue/factory.go 0.00% <0.00%> (ø)

... and 8 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 73e6dbc...7f86ea2. Read the comment docs.

@coveralls
Copy link

coveralls commented May 8, 2024

Pull Request Test Coverage Report for Build 018f59b5-6531-41ed-9764-e612b3e75900

Details

  • 38 of 38 (100.0%) changed or added relevant lines in 3 files are covered.
  • 33 unchanged lines in 11 files lost coverage.
  • Overall coverage increased (+0.02%) to 68.521%

Files with Coverage Reduction New Missed Lines %
common/task/weighted_round_robin_task_scheduler.go 2 89.05%
common/cache/lru.go 2 92.89%
service/history/task/transfer_active_task_executor.go 2 72.9%
service/matching/taskListManager.go 2 81.16%
service/matching/matcher.go 2 90.72%
common/membership/hashring.go 2 84.69%
common/persistence/historyManager.go 2 66.67%
service/history/task/transfer_standby_task_executor.go 3 86.21%
common/persistence/statsComputer.go 3 96.07%
service/history/task/fetcher.go 4 85.05%
Totals Coverage Status
Change from base Build 018f5920-e167-45cd-8901-b0a5fa3bbbfc: 0.02%
Covered Lines: 100669
Relevant Lines: 146916

💛 - Coveralls


//go:generate mockgen -package $GOPACKAGE -source $GOFILE -destination factory_mock.go -self_package github.com/uber/cadence/service/history/queue

type ProcessorFactory interface {
Copy link
Member

@Groxx Groxx May 8, 2024

Choose a reason for hiding this comment

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

tbh it feels a little odd that these are bundled together. is there any reason other than convenience, since all three are used by the same caller(s?)?

but I don't have much context for the full refactor-and-test planned here, possibly I'm just missing something obvious.

Copy link
Member Author

Choose a reason for hiding this comment

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

I haven't made my mind how the final objects/interfaces will look like after refactoring but I am also considering the option where there's just single "Queue" object that history interacts with. Based on the task type, domain and other attributes that top level object decides where to route the request.

@taylanisikdemir taylanisikdemir merged commit 9ede952 into cadence-workflow:master May 8, 2024
18 checks passed
@taylanisikdemir taylanisikdemir deleted the taylan/history_engine_test branch May 8, 2024 20:08
timl3136 pushed a commit to timl3136/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.

4 participants