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

Merge Activity and Decision matching tests #5186

Merged
merged 1 commit into from
Apr 4, 2023

Conversation

Shaddoll
Copy link
Contributor

@Shaddoll Shaddoll commented Apr 4, 2023

What changed?
Merge Activity and Decision matching tests

Why?
To reuse test logic for activity and decision tasks

How did you test it?
the test

Potential risks

Release notes

Documentation Changes

@coveralls
Copy link

coveralls commented Apr 4, 2023

Pull Request Test Coverage Report for Build 01874e16-5537-4e06-8f01-b13ef79ff223

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 54 unchanged lines in 11 files lost coverage.
  • Overall coverage decreased (-0.02%) to 57.081%

Files with Coverage Reduction New Missed Lines %
client/history/client.go 2 38.1%
client/history/metricClient.go 2 45.3%
service/history/handler.go 2 47.15%
service/matching/taskListManager.go 3 81.15%
common/persistence/serialization/parser.go 4 62.41%
common/persistence/serialization/thrift_decoder.go 4 53.06%
service/frontend/workflowHandler.go 4 59.86%
service/history/task/transfer_standby_task_executor.go 4 86.4%
common/persistence/sql/workflowStateMaps.go 8 81.25%
service/history/historyEngine.go 9 68.64%
Totals Coverage Status
Change from base Build 01874884-43c6-4cf8-b6b8-ecc8c5686563: -0.02%
Covered Lines: 85251
Relevant Lines: 149351

💛 - Coveralls

service/matching/matchingEngine_test.go Show resolved Hide resolved
ScheduleID: scheduleID,
ActivityID: activityID,
ActivityType: activityTypeName,
for i := int64(0); i < throttledTaskCount; i++ {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not imediately clear what's going on here without being pretty steeped in the polling code. Do you mind either mind putting a comment or wrapping this in some function which indicates the intent. It's not immediately clear to me what throttledTaskCount is intended to be used for?

@davidporter-id-au
Copy link
Contributor

Nit: Non-blocking comment - More generally, I know the temptation for putting particularly messy code with lots of duplication into a way which is DRYer is tempting, and for test-code it's probably ok, but having code covered by switch or IF statements does make it tricky to follow.

In this instance you're starting out with

if task == Decision {
 ... 
} else {
 ...
}

which is pretty simple, and I think works here. But if there are parts that start to get shared it risks becoming quite hard to follow. I'd lean towards just leaving the code duplicated.

@Shaddoll Shaddoll merged commit d2f9ec6 into uber:master Apr 4, 2023
@Shaddoll Shaddoll deleted the refactor-matching-test2 branch April 4, 2023 21:29
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