[testify-expert] Improve Test Quality: pkg/workflow/is_task_job_needed_test.go

[github-actions]

Current State

• File: pkg/workflow/is_task_job_needed_test.go (56 lines)
• Source pair: pkg/workflow/compiler_jobs.go — func (c *Compiler) isActivationJobNeeded() bool (line 23)
• Test count: 1 test function (TestIsActivationJobNeeded) with 4 subtests, all exercising the same method
• Assertions: 0 testify calls — uses raw if + t.Errorf
• LOC of source func: 8 lines (the function currently just returns true unconditionally)

Strengths

• Uses t.Run subtests with descriptive names.
• Correctly documents (in comments) the historical behavior being tested (permission checks, If condition, etc.).

Prioritized Improvements

1. Missing / high-value tests

isActivationJobNeeded() is now a constant-true function — the 4 subtests are functionally identical (each just asserts true) and provide no differentiating coverage. Since WorkflowData fields (Roles, If) are constructed but never passed into the call (note the var _ = data no-op), the subtests don't actually test any conditional logic. Recommend either:

• Collapsing to a single TestIsActivationJobNeeded_AlwaysTrue test (since the function is unconditional), or
• If the intent is to guard against future regressions where the function again depends on WorkflowData, add a TODO/skip explaining current behavior and keep minimal coverage.

Also consider adding coverage for the neighboring functions in the same file's source (referencesCustomJobOutputs, jobDependsOnPreActivation) — already covered elsewhere in compiler_jobs_test.go and custom_job_condition_test.go, so no gap there, but worth confirming no duplication drift.

2. Testify assertion upgrades

Replace manual if !cond { t.Errorf(...) } with assert.True(t, ...) / require.True(t, ...) for clearer failure diagnostics and to align with the rest of the pkg/workflow package (see compiler_jobs_test.go, which already imports testify/assert and testify/require).

Before / After

Before:

func TestIsActivationJobNeeded(t *testing.T) {
	compiler := NewCompiler()

	t.Run("no_conditions", func(t *testing.T) {
		data := &WorkflowData{
			Roles: []string{"all"},
		}
		if !func() bool {
			var _ = data
			return compiler.isActivationJobNeeded()
		}() {
			t.Errorf("Expected isActivationJobNeeded to be true - activation job is always needed for timestamp check")
		}
	})
	...
}

After:

package workflow

import (
	"testing"

	"github.com/stretchr/testify/assert"
)

func TestIsActivationJobNeeded_AlwaysTrue(t *testing.T) {
	compiler := NewCompiler()

	assert.True(t, compiler.isActivationJobNeeded(),
		"activation job must always run to perform the timestamp check")
}

3. Table-driven refactor

If the goal is to keep multiple named scenarios for documentation purposes (rather than dead-code closures), convert to a table so scenario names and expectations are explicit and don't rely on discarded WorkflowData values:

Example

func TestIsActivationJobNeeded(t *testing.T) {
	compiler := NewCompiler()

	tests := []struct {
		name string
		want bool
	}{
		{"no_conditions", true},
		{"if_condition_present", true},
		{"default_permission_check", true},
		{"permission_check_not_needed", true},
	}

	for _, tt := range tests {
		t.Run(tt.name, func(t *testing.T) {
			assert.Equal(t, tt.want, compiler.isActivationJobNeeded())
		})
	}
}

Note: since isActivationJobNeeded() takes no arguments, this table doesn't add real behavioral coverage — this refactor is only worth doing if the maintainers plan to reintroduce parameterization (e.g., passing *WorkflowData) in the near future. Otherwise, prefer the single-test approach in item #2.

4. Organization / readability

• Remove the unused var _ = data no-op pattern and unused WorkflowData construction in each subtest — it implies the function depends on data, which is misleading since isActivationJobNeeded() takes no parameters.
• Add a doc comment on the test function explaining that isActivationJobNeeded() currently always returns true (per the source comment), so future readers understand why the test doesn't branch on conditions.
• Consider renaming file/test to TestIsActivationJobNeeded_AlwaysTrue or adding a comment that ties test intent to the source's documented behavior in compiler_jobs.go lines 24-30.

Acceptance Checklist

• Replace t.Errorf boolean checks with assert.True/require.True from testify
• Remove misleading no-op WorkflowData construction and var _ = data closures
• Add a short doc comment referencing the "always true" behavior documented in compiler_jobs.go
• (Optional) Collapse redundant subtests or convert to a table if parameterization is planned
• make test-unit passes after changes

Generated by 🧪 Daily Testify Uber Super Expert · auto · 20.3 AIC · ⌖ 4.16 AIC · ⊞ 7.5K · ◷

• expires on Aug 19, 2026, 10:09 AM UTC-08:00

[github-actions]

🍪 Issue Monster selected this for Copilot

I've identified this issue as a good candidate for automated resolution and requested assignment to the Copilot coding agent.

If assignment succeeds, the Copilot coding agent will analyze the issue and create a pull request with the fix.

Om nom nom! 🍪

🍪 Om nom nom by Issue Monster · gpt54 · 2.08 AIC · ⌖ 6.07 AIC · ⊞ 9.5K · ◷