-
Notifications
You must be signed in to change notification settings - Fork 96
Add comprehensive test coverage for compiler_orchestrator_workflow.go #14448
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
Add comprehensive test coverage for compiler_orchestrator_workflow.go #14448
Conversation
… (462 new lines, 11 new tests) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Adds additional unit tests to improve coverage for the workflow compilation orchestrator, focusing on end-to-end ParseWorkflowFile behavior and several orchestration helpers involved in YAML extraction and import merging.
Changes:
- Adds new
ParseWorkflowFiletests covering phase data flow, full frontmatter parsing, and error propagation paths. - Adds targeted tests for orchestration helpers (YAML section extraction, job/service/step merging edge cases).
- Adds a field-mapping test for
buildInitialWorkflowDatato validate struct composition.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // TestParseWorkflowFile_BashToolValidationBeforeDefaults tests bash validation occurs before defaults | ||
| func TestParseWorkflowFile_BashToolValidationBeforeDefaults(t *testing.T) { | ||
| tmpDir := testutil.TempDir(t, "bash-validation") | ||
|
|
||
| // Test that bash validation happens before applyDefaults | ||
| testContent := `--- | ||
| on: push | ||
| engine: copilot | ||
| tools: | ||
| bash: [] | ||
| --- | ||
|
|
||
| # Test Workflow | ||
| ` | ||
|
|
||
| testFile := filepath.Join(tmpDir, "bash-test.md") | ||
| require.NoError(t, os.WriteFile(testFile, []byte(testContent), 0644)) | ||
|
|
||
| compiler := NewCompiler() | ||
| workflowData, err := compiler.ParseWorkflowFile(testFile) | ||
|
|
||
| // Empty bash array should be valid (nil bash would be converted to defaults) | ||
| require.NoError(t, err) | ||
| require.NotNil(t, workflowData) |
Copilot
AI
Feb 8, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The scenario here (tools.bash: []) doesn’t actually verify that bash validation runs before applyDefaults. It will pass regardless of ordering because an explicit empty list parses as non-nil Bash config. To test the intended behavior, use the anonymous/null syntax (bash: with no value) and assert that ParseWorkflowFile returns the expected validateBashToolConfig error (which would otherwise be masked by defaults), or rename the test to reflect that it’s only asserting empty list is allowed.
| // TestMergeJobsFromYAMLImports_PreservesJobOrder tests job merge preserves main job definitions | ||
| func TestMergeJobsFromYAMLImports_PreservesJobOrder(t *testing.T) { | ||
| compiler := NewCompiler() | ||
|
|
||
| mainJobs := map[string]any{ | ||
| "job-a": map[string]any{"runs-on": "ubuntu-latest"}, | ||
| "job-b": map[string]any{"runs-on": "ubuntu-latest"}, | ||
| } | ||
|
|
||
| importedJobsJSON := `{"job-c": {"runs-on": "ubuntu-latest"}} | ||
| {"job-d": {"runs-on": "macos-latest"}}` | ||
|
|
||
| result := compiler.mergeJobsFromYAMLImports(mainJobs, importedJobsJSON) | ||
|
|
||
| assert.Len(t, result, 4) | ||
| // Verify all jobs present | ||
| assert.Contains(t, result, "job-a") | ||
| assert.Contains(t, result, "job-b") | ||
| assert.Contains(t, result, "job-c") | ||
| assert.Contains(t, result, "job-d") | ||
| } |
Copilot
AI
Feb 8, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test name/comment claims job order preservation, but mergeJobsFromYAMLImports returns a map, and Go map iteration/insertion order is not deterministic. The assertions only check presence/len, not order. Consider renaming to reflect what’s actually verified (e.g., that imported jobs are added and main jobs aren’t overridden), or change the test to validate deterministic output at the YAML rendering layer if order is a real requirement.
The workflow compilation orchestrator (536 lines) coordinates all compilation phases but lacked comprehensive test coverage. Added 11 tests (462 lines) covering phase execution, error propagation, data merging, and edge cases.
New Test Coverage
Phase Execution & Data Flow
Error Propagation
Data Merging & Composition
Edge Cases
Test Pattern
Test file: 1,457 lines (43 tests), 2.72:1 test-to-source ratio
Original prompt
This section details on the original issue you should resolve
<issue_title>[Code Quality] Create comprehensive test file for compiler_orchestrator_workflow.go</issue_title>
<issue_description>## Description
The file
pkg/workflow/compiler_orchestrator_workflow.go(536 lines) completely lacks test coverage despite coordinating critical compilation phases. This orchestration logic handles workflow parsing, engine setup, tool configuration, and step merging - all essential for correct compilation.Current State:
Why Critical:
Suggested Changes
Create
pkg/workflow/compiler_orchestrator_workflow_test.gowith focus on:1. Phase Execution Validation
Test that compilation phases execute in correct order:
parseFrontmatterSection→setupEngineAndImports→processToolsAndMarkdown→buildInitialWorkflowData2. Error Propagation Tests
Test that errors from any phase properly propagate up:
3. Result Struct Composition
Test that intermediate results are correctly composed:
4. Edge Cases
Files Affected
pkg/workflow/compiler_orchestrator_workflow_test.go(target: 400-500 lines)pkg/workflow/compiler_orchestrator_workflow.go(536 lines)Test Coverage Goals
Success Criteria
t.Run()%wwrapping (5 instances exist)make test-unit)Implementation Approach
Phase 1: Setup (30 min)
Phase 2: Core Tests (2-3 hours)
Phase 3: Edge Cases (1-2 hours)
Phase 4: Validation (30 min)
make test-unit- verify all passgo test -covermake agent-finishfor full validationSource
Extracted from Daily Compiler Code Quality Report discussion github/gh-aw#14370
Quality Assessment:
Priority
High - Critical orchestration logic should have comprehensive test coverage to prevent regression bugs
Comments on the Issue (you are @copilot in this section)
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.