Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions internal/actions/test.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,16 @@ func Test(ctx context.Context) error {
if providedTargetName := environment.SpecifiedTarget(); providedTargetName != "" && os.Getenv("GITHUB_EVENT_NAME") == "workflow_dispatch" {
testedTargets = append(testedTargets, providedTargetName)
}

// If no target is specified via workflow dispatch, check for testing enabled targets and pick the first one
if len(testedTargets) == 0 {
for name, target := range wf.Targets {
if target.Testing != nil && target.Testing.Enabled != nil && *target.Testing.Enabled {
testedTargets = append(testedTargets, name)
break // Pick the first one with testing enabled
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll admit I'm struggling with the best behavior choice here. I see choosing the first but that also feels like a random guess in a multi-target setup. Should we run all instead if undefined?

I think we should make a change like this either which way rather than doing nothing, but we can also improve that in the generated GHA workflow file that lands in customer repos to either require or default to better target input selection during workflow dispatch. We should be writing per-target test workflows with speakeasy configure testing so it'd be just a matter of adding Default string yaml:"default" in sdk-gen-config Target type then setting it appropriately in the CLI logic that writes out the files.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I totally follow what you've said. However, there is a ticket connected to this PR. We can discuss during triage the appropriate solution and with whoever will test and finish this off.

}
}
}

var prNumber *int
targetLockIDs := make(map[string]string)
Expand Down
155 changes: 155 additions & 0 deletions internal/actions/test_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,155 @@
package actions

import (
"os"
"testing"

"github.com/speakeasy-api/sdk-gen-config/workflow"
)

func TestTest_InitializeTestedTargetsFromWorkflow(t *testing.T) {
// Save original environment variables
originalSpecifiedTarget := os.Getenv("INPUT_TARGET")
originalEventName := os.Getenv("GITHUB_EVENT_NAME")
defer func() {
os.Setenv("INPUT_TARGET", originalSpecifiedTarget)
os.Setenv("GITHUB_EVENT_NAME", originalEventName)
}()

// Mock workflow with testing enabled/disabled targets
enabledTrue := true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really wish Go would support inline pointer value declarations or at least standard library the func Pointer[T any](v T) *T { return &v } function. 😭

enabledFalse := false

tests := []struct {
name string
workflow *workflow.Workflow
specifiedTarget string
eventName string
expectedTargets []string
expectedSkip bool
}{
{
name: "workflow dispatch with specified target",
workflow: &workflow.Workflow{
Targets: map[string]workflow.Target{
"go": {
Testing: &workflow.Testing{Enabled: &enabledTrue},
},
"python": {
Testing: &workflow.Testing{Enabled: &enabledFalse},
},
},
},
specifiedTarget: "go",
eventName: "workflow_dispatch",
expectedTargets: []string{"go"},
expectedSkip: false,
},
{
name: "no specified target, picks first testing enabled",
workflow: &workflow.Workflow{
Targets: map[string]workflow.Target{
"python": {
Testing: &workflow.Testing{Enabled: &enabledFalse},
},
"go": {
Testing: &workflow.Testing{Enabled: &enabledTrue},
},
"typescript": {
Testing: &workflow.Testing{Enabled: &enabledTrue},
},
},
},
specifiedTarget: "",
eventName: "push",
expectedTargets: []string{"go"}, // Should pick first one with testing enabled
expectedSkip: false,
},
{
name: "no specified target, no testing enabled targets",
workflow: &workflow.Workflow{
Targets: map[string]workflow.Target{
"go": {
Testing: &workflow.Testing{Enabled: &enabledFalse},
},
"python": {
Testing: nil, // No testing config
},
},
},
specifiedTarget: "",
eventName: "push",
expectedTargets: []string{},
expectedSkip: true,
},
{
name: "no specified target, nil testing config",
workflow: &workflow.Workflow{
Targets: map[string]workflow.Target{
"go": {
Testing: nil,
},
"python": {
Testing: &workflow.Testing{Enabled: nil},
},
},
},
specifiedTarget: "",
eventName: "push",
expectedTargets: []string{},
expectedSkip: true,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
// Set environment variables for test
os.Setenv("INPUT_TARGET", tt.specifiedTarget)
os.Setenv("GITHUB_EVENT_NAME", tt.eventName)

// Test the target selection logic
var testedTargets []string

// Simulate the logic from Test function
if providedTargetName := tt.specifiedTarget; providedTargetName != "" && os.Getenv("GITHUB_EVENT_NAME") == "workflow_dispatch" {
testedTargets = append(testedTargets, providedTargetName)
}

// If no target is specified via workflow dispatch, check for testing enabled targets and pick the first one
if len(testedTargets) == 0 {
for name, target := range tt.workflow.Targets {
if target.Testing != nil && target.Testing.Enabled != nil && *target.Testing.Enabled {
testedTargets = append(testedTargets, name)
break // Pick the first one with testing enabled
}
}
}

// Verify results
if tt.expectedSkip && len(testedTargets) != 0 {
t.Errorf("Expected no targets to be selected, but got: %v", testedTargets)
}

if !tt.expectedSkip {
if len(testedTargets) != len(tt.expectedTargets) {
t.Errorf("Expected %d targets, got %d: %v", len(tt.expectedTargets), len(testedTargets), testedTargets)
}

// For the case where we pick the first testing-enabled target,
// we need to check that it's one of the expected ones
if len(tt.expectedTargets) > 0 && len(testedTargets) > 0 {
found := false
for _, expected := range tt.expectedTargets {
if testedTargets[0] == expected {
found = true
break
}
}
if !found {
t.Errorf("Expected target %v to be one of %v", testedTargets[0], tt.expectedTargets)
}
}
}
})
}
}
Loading