[internal] Improve code coverage of internal_task_pollers.go #1373

3vilhamster · 2024-10-30T17:09:59Z

What changed?
Refactored task pollers and improve code coverage.

Why?
Improvign code coverage.

How did you test it?
unit tests

Potential risks

codecov · 2024-10-31T20:58:34Z

Codecov Report

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

Project coverage is 77.72%. Comparing base (f21e350) to head (d36df23).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
internal/internal_task_pollers.go	95.34%	2 Missing and 2 partials ⚠️

Additional details and impacted files

Files with missing lines	Coverage Δ
internal/internal_public.go	`100.00% <ø> (ø)`
internal/internal_task_pollers.go	`82.64% <95.34%> (+5.45%)`	⬆️

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 f21e350...d36df23. Read the comment docs.

Makefile

codecov.yml

internal/internal_public.go

internal/internal_task_pollers.go

Groxx

Only minor stuff to change I think:

just add the dependencies so it keeps working. I'll switch it to a grep afterward.
opts maybe? I kinda see why you did it, just not sure it's worth it
tracelog needs an err check at least, probably minor changes
minor test precision since "panics" is extremely broad about what it's checking for

otherwise LGTM, breaking that up seems like a solid improvement 👍

Groxx · 2024-11-01T01:04:52Z

internal/internal_task_pollers.go

@@ -365,10 +372,10 @@ func (wtp *workflowTaskPoller) processWorkflowTask(task *workflowTask) error {
 		if completedRequest == nil && err == nil {
 			return nil
 		}
-		if _, ok := err.(decisionHeartbeatError); ok {
+		if errors.As(err, new(*decisionHeartbeatError)) {


yea, looks good - it's only returned from one place right now, and no wrapping, so this won't change behavior 👍

This was not working before at all.
The error was returned as &decicisionHeartbeatError so type assertion err.(decisionHeartbeatError) was always false.

ah, good point. and good fix 👍

internal/internal_task_pollers.go

internal/internal_task_pollers_test.go

Co-authored-by: Steven L <[email protected]>

Does a couple things: - `make generate` now infers files that might affect generation. This should work as long as we keep simple codegen (the server has moved beyond this, sadly). - `make generate` now forces re-generation regardless of need, because it's a nice workaround for dependency issues like happened in uber-go#1373 - `make test` and related targets now go-generate and fmt before running tests. Slower, but you can't forget to re-generate, and they're slow already so it's probably fine.

Does a couple things: - `make $(BUILD)/generate` now infers files that might affect generation. This should work as long as we keep simple codegen (the server has moved beyond this, sadly). - `make generate` now forces re-generation regardless of need, because it's a nice workaround for dependency issues like happened in #1373 - `make test` and related targets now go-generate and fmt before running tests. Slower, but you can't forget to re-generate, and they're slow already so it's probably fine.

3vilhamster requested review from Groxx, shijiesheng, jakobht, dkrotx, taylanisikdemir and demirkayaender as code owners October 30, 2024 17:10

3vilhamster force-pushed the internal-code-coverage branch 2 times, most recently from 6427c91 to c189829 Compare October 31, 2024 20:47