uber-go / cadence-client

Framework for authoring workflows and activities running on top of the Cadence orchestration engine.
https://cadenceworkflow.io
MIT License
345 stars 131 forks source link

fmt, removing globals from tests, scoping test logs #1142

Closed Groxx closed 2 years ago

Groxx commented 2 years ago

... or nearly. TestWorkerOptionDefaults currently produces a log because we have an info-level log when no logger is set, and... I think that makes sense.

=== RUN   TestWorkerOptionDefaults
{"level":"info","ts":"2021-10-11T16:42:58.778-0700","caller":"internal/internal_worker.go:223","msg":"No logger configured for cadence worker. Created default one."}
{"level":"info","ts":"2021-10-11T16:42:58.779-0700","caller":"internal/internal_worker.go:227","msg":"No metrics scope configured for cadence worker. Use NoopScope as default."}

A better behavior may be to wait to produce that log until the worker is started, but that's a bit out of scope for this PR + we may not want to do that anyway.

I also got rid of a couple suspicious globals that have no need to be globals, and added a bit of documentation about what the heck they were doing anyway.

coveralls commented 2 years ago

Pull Request Test Coverage Report for Build 0299e6e3-ee38-4240-abdf-8ef6830c91be


Changes Missing Coverage Covered Lines Changed/Added Lines %
internal/session.go 0 1 0.0%
internal/workflow.go 0 1 0.0%
mocks/Client.go 0 1 0.0%
internal/jwt_authorization.go 18 22 81.82%
internal/common/util/rsa.go 0 29 0.0%
<!-- Total: 299 335 89.25% -->
Files with Coverage Reduction New Missed Lines %
internal/internal_workflow_testsuite.go 3 86.98%
<!-- Total: 3 -->
Totals Coverage Status
Change from base Build 6267bf7d-33d4-47f4-be49-1f030f8e8ff0: 0.0%
Covered Lines: 12177
Relevant Lines: 19275

💛 - Coveralls
Groxx commented 2 years ago

Well. Unfortunately this exposes a race due to our incorrect shutdown logic, since we're using testing.T logs more :\ (thanks @demirkayaender for discovering)

In testing.go there's this note:

// Do not lock t.done to allow race detector to detect race in case
// the user does not appropriately synchronizes a goroutine.
t.done = true
if t.parent != nil && atomic.LoadInt32(&t.hasSub) == 0 {
    t.setRan()
}

If it happens too often, we can revert. Otherwise tbh I'd like to treat this as motivation to fix our leaky and incorrect shutdown logic.