test: integration tests - #495
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 Jules Review
This pull request successfully introduces a highly robust and extensible execution strategy system (featuring Standard, Dry-Run, and Retry strategies) along with a configurable TaskRunner API. The architectural design is clean, aligns perfectly with the project requirements, and is backed by exceptional test coverage (reaching 99.2% on internal/runner package), presenting zero blocking issues.
Session: 4853057594084152356
|
|
||
| // Wait before retrying (with context support) | ||
| if attempt < s.maxRetries { | ||
| select { |
There was a problem hiding this comment.
Severity:
Using time.After in a loop/select statement can lead to transient timer or memory leaks if the context is cancelled before the backoff timer fires. Instead, use time.NewTimer and ensure it is stopped correctly via defer timer.Stop().
🤖 Prompt for Agents
In go/internal/runner/retry_strategy.go at line 46, refactor the time.After call in the select block to use time.NewTimer(s.backoff) instead, and ensure to call timer.Stop() when exiting or on context cancellation to prevent timer resource leaks.
| strategy ExecutionStrategy[T] | ||
| } | ||
|
|
||
| func (t *wrappedTask[T]) Run(ctx context.Context, sharedState T) TaskResult { |
There was a problem hiding this comment.
Severity:
If TaskRunner is zero-initialized directly (e.g. tr := &runner.TaskRunner[struct{}]{}) rather than via NewTaskRunner, tr.strategy will be nil, causing a nil-pointer panic when executing tasks. Adding a fallback to StandardStrategy in wrappedTask.Run is highly recommended for zero-value safety.
🤖 Prompt for Agents
In go/internal/runner/task_runner.go at line 66, add a check inside wrappedTask.Run to fallback to direct execution (t.inner.Run) if t.strategy is nil to prevent panics during zero-value initialization of TaskRunner.
1 participant
This pull request introduces a new, extensible execution strategy system for task execution, adds dry-run and retry support, and significantly improves test coverage for these new features. It also introduces a configurable
TaskRunnerAPI for orchestrating task graphs with pluggable strategies and options. Minor internal improvements and tests for edge cases in the executor are also included.Execution Strategy System and Extensibility
ExecutionStrategyinterface and theStandardStrategyimplementation, allowing flexible wrapping of task execution with behaviors such as retries or dry-run mode (execution_strategy.go).DryRunStrategywhich simulates task execution by returning success immediately, useful for validation or "what-if" runs (dryrun_strategy.go).RetryStrategyandNewRetryStrategy, which retries failed executions with configurable attempts and backoff, respecting context cancellation (retry_strategy.go).Task Runner API and Configuration
TaskRunnertype, a central orchestrator for executing task graphs with configurable concurrency, plugins, and pluggable execution strategies. Provided functional options for configuration and a method to set the execution strategy (task_runner.go).Testing and Reliability
execution_strategy_test.go,retry_strategy_test.go) [1] [2].executor_internal_test.go).simpleTaskto verify cancellation and success (main_test.go).Internal Improvements
handleResultinexecutor.goto ignore returned values fromMarkDependencyFailedandMarkCompleted, clarifying intent and reducing unused variable warnings.