-
Notifications
You must be signed in to change notification settings - Fork 28
Fix timeout-minutes schema documentation: 20 minutes, not 15 #4030
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,172 @@ | ||
| package workflow | ||
|
|
||
| import ( | ||
| "os" | ||
| "path/filepath" | ||
| "strings" | ||
| "testing" | ||
|
|
||
| "github.com/githubnext/gh-aw/pkg/constants" | ||
| ) | ||
|
|
||
| func TestDefaultTimeoutMinutesApplied(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| frontmatter string | ||
| expectedTimeout int | ||
| description string | ||
| }{ | ||
| { | ||
| name: "no timeout specified - should use default", | ||
| frontmatter: `--- | ||
| on: workflow_dispatch | ||
| permissions: | ||
| contents: read | ||
| engine: copilot | ||
| ---`, | ||
| expectedTimeout: constants.DefaultAgenticWorkflowTimeoutMinutes, | ||
| description: "When timeout-minutes is not specified, default should be applied", | ||
| }, | ||
| { | ||
| name: "explicit timeout specified - should use explicit value", | ||
| frontmatter: `--- | ||
| on: workflow_dispatch | ||
| permissions: | ||
| contents: read | ||
| timeout-minutes: 30 | ||
| engine: copilot | ||
| ---`, | ||
| expectedTimeout: 30, | ||
| description: "When timeout-minutes is explicitly specified, that value should be used", | ||
| }, | ||
| { | ||
| name: "deprecated timeout_minutes specified - should use that value", | ||
| frontmatter: `--- | ||
| on: workflow_dispatch | ||
| permissions: | ||
| contents: read | ||
| timeout_minutes: 45 | ||
| engine: copilot | ||
| ---`, | ||
| expectedTimeout: 45, | ||
| description: "When deprecated timeout_minutes is specified, that value should be used", | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| // Create temporary directory for test | ||
| tmpDir, err := os.MkdirTemp("", "timeout-default-test") | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| defer os.RemoveAll(tmpDir) | ||
|
|
||
| // Create test workflow file | ||
| testContent := tt.frontmatter + "\n\n# Test Workflow\n\nTest workflow for timeout-minutes default behavior.\n" | ||
| testFile := filepath.Join(tmpDir, "test-workflow.md") | ||
| if err := os.WriteFile(testFile, []byte(testContent), 0644); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| // Compile the workflow | ||
| compiler := NewCompiler(false, "", "test") | ||
| err = compiler.CompileWorkflow(testFile) | ||
| if err != nil { | ||
| t.Fatalf("Failed to compile workflow: %v", err) | ||
| } | ||
|
|
||
| // Read the generated lock file | ||
| lockFile := strings.TrimSuffix(testFile, ".md") + ".lock.yml" | ||
| lockContent, err := os.ReadFile(lockFile) | ||
| if err != nil { | ||
| t.Fatalf("Failed to read lock file: %v", err) | ||
| } | ||
|
|
||
| // Check that the expected timeout is present in the lock file | ||
| expectedTimeoutStr := "timeout-minutes: " + string(rune(tt.expectedTimeout+'0')) | ||
| if tt.expectedTimeout >= 10 { | ||
| expectedTimeoutStr = "timeout-minutes: " + intToString(tt.expectedTimeout) | ||
| } | ||
|
|
||
| if !strings.Contains(string(lockContent), expectedTimeoutStr) { | ||
| t.Errorf("%s\nExpected timeout-minutes: %d in compiled workflow, but not found\nLock file content:\n%s", | ||
| tt.description, tt.expectedTimeout, string(lockContent)) | ||
| } | ||
|
|
||
| // Verify the timeout appears in the execution step (not just anywhere in the file) | ||
| // The timeout should be in a step, not in comments | ||
| lines := strings.Split(string(lockContent), "\n") | ||
| foundTimeoutInStep := false | ||
| for _, line := range lines { | ||
| trimmed := strings.TrimSpace(line) | ||
| // Skip comments | ||
| if strings.HasPrefix(trimmed, "#") { | ||
| continue | ||
| } | ||
| if strings.Contains(trimmed, expectedTimeoutStr) { | ||
| foundTimeoutInStep = true | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if !foundTimeoutInStep { | ||
| t.Errorf("%s\nExpected timeout-minutes: %d in a workflow step (not in comments)\nLock file content:\n%s", | ||
| tt.description, tt.expectedTimeout, string(lockContent)) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| // Helper function to convert int to string | ||
| func intToString(n int) string { | ||
| if n == 0 { | ||
| return "0" | ||
| } | ||
| if n < 0 { | ||
| return "-" + intToString(-n) | ||
| } | ||
| result := "" | ||
| for n > 0 { | ||
| result = string(rune('0'+n%10)) + result | ||
| n /= 10 | ||
| } | ||
| return result | ||
| } | ||
|
Comment on lines
+121
to
+135
|
||
|
|
||
| func TestDefaultTimeoutMinutesConstantValue(t *testing.T) { | ||
| // This test ensures the constant is set to the expected value | ||
| // If this test fails, it means the constant was changed and documentation | ||
| // should be updated accordingly | ||
| expectedDefault := 20 | ||
| if constants.DefaultAgenticWorkflowTimeoutMinutes != expectedDefault { | ||
| t.Errorf("DefaultAgenticWorkflowTimeoutMinutes constant is %d, but test expects %d. "+ | ||
| "If you changed the constant, please update the schema documentation in pkg/parser/schemas/main_workflow_schema.json", | ||
| constants.DefaultAgenticWorkflowTimeoutMinutes, expectedDefault) | ||
| } | ||
| } | ||
|
|
||
| func TestSchemaDocumentationMatchesConstant(t *testing.T) { | ||
| // Read the schema file | ||
| schemaPath := filepath.Join("..", "parser", "schemas", "main_workflow_schema.json") | ||
| schemaContent, err := os.ReadFile(schemaPath) | ||
| if err != nil { | ||
| t.Fatalf("Failed to read schema file: %v", err) | ||
| } | ||
|
|
||
| // Check that the schema mentions the correct default value | ||
| expectedText := "Defaults to 20 minutes for agentic workflows" | ||
| if !strings.Contains(string(schemaContent), expectedText) { | ||
| t.Errorf("Schema documentation does not mention the correct default timeout.\n"+ | ||
| "Expected to find: %q\n"+ | ||
| "Please update the timeout-minutes description in %s to match DefaultAgenticWorkflowTimeoutMinutes constant (%d minutes)", | ||
| expectedText, schemaPath, constants.DefaultAgenticWorkflowTimeoutMinutes) | ||
| } | ||
|
|
||
| // Count occurrences - should appear at least twice (timeout-minutes and timeout_minutes deprecated) | ||
| occurrences := strings.Count(string(schemaContent), expectedText) | ||
| if occurrences < 2 { | ||
| t.Errorf("Expected to find at least 2 occurrences of %q in schema (for timeout-minutes and timeout_minutes fields), but found %d", | ||
| expectedText, occurrences) | ||
| } | ||
| } | ||
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 logic for building
expectedTimeoutStris flawed. Line 87 attempts to convert an integer to a string usingstring(rune(tt.expectedTimeout+'0')), which works incorrectly for multi-digit numbers. For example, withexpectedTimeout=20, this would computestring(rune(68))="D", producing"timeout-minutes: D".While the code on lines 88-90 fixes this for values >= 10 by calling
intToString, the initial assignment on line 87 is incorrect and unnecessary.Recommended fix: Use Go's standard library
strconv.Itoa()instead:This eliminates the need for the custom
intToStringhelper function (lines 121-135) and the conditional check on line 88.