Skip to content

Commit 203fb3f

Browse files
authored
refactor(component,generic,http): move test functions to test files and improve code legibility (#1131)
Because - PR #1121 comments requested moving test-specific functions out of production code to improve code organization - Nested if-else structures in test code reduced readability and could be simplified with early returns This commit - Moves `NewTestURLValidator` from `validator.go` to `validator_test.go` to keep test utilities in test files - Moves `InitForTest` from `main.go` to `validator_test.go` to separate test-specific component initialization from production code - Refactors nested if-else structure in validator test (lines 90-99) using early returns to improve code legibility - Removes test functions from production code, reducing binary size and improving maintainability
1 parent 4571d6a commit 203fb3f

File tree

3 files changed

+34
-31
lines changed

3 files changed

+34
-31
lines changed

pkg/component/generic/http/v0/main.go

Lines changed: 0 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -83,21 +83,6 @@ func Init(bc base.Component) *component {
8383
return comp
8484
}
8585

86-
// InitForTest creates a component instance for testing with configurable validation
87-
// whitelist: URLs to allow (nil/empty = allow all external URLs)
88-
// allowLocalhost: whether to allow localhost/127.x.x.x URLs
89-
func InitForTest(bc base.Component, whitelist []string, allowLocalhost bool) *component {
90-
c := &component{
91-
Component: bc,
92-
urlValidator: NewTestURLValidator(whitelist, allowLocalhost),
93-
}
94-
err := c.LoadDefinition(definitionYAML, setupYAML, tasksYAML, nil, nil)
95-
if err != nil {
96-
panic(err)
97-
}
98-
return c
99-
}
100-
10186
func (c *component) CreateExecution(x base.ComponentExecution) (base.IExecution, error) {
10287

10388
// We may have different url in batch.

pkg/component/generic/http/v0/validator.go

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -28,15 +28,6 @@ func NewURLValidator() URLValidator {
2828
return &urlValidator{allowPrivateIPs: false}
2929
}
3030

31-
// NewTestURLValidator creates a validator for testing
32-
func NewTestURLValidator(whitelistedEndpoints []string, allowLocalhost bool) URLValidator {
33-
return &urlValidator{
34-
whitelistedEndpoints: whitelistedEndpoints,
35-
allowLocalhost: allowLocalhost,
36-
allowPrivateIPs: true, // Test mode allows external URLs by default
37-
}
38-
}
39-
4031
// ValidateInput implements the consolidated validation logic
4132
func (v *urlValidator) ValidateInput(input *httpInput) error {
4233
if input == nil {

pkg/component/generic/http/v0/validator_test.go

Lines changed: 34 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,30 @@ import (
88
"github.com/instill-ai/pipeline-backend/pkg/component/base"
99
)
1010

11+
// NewTestURLValidator creates a validator for testing
12+
func NewTestURLValidator(whitelistedEndpoints []string, allowLocalhost bool) URLValidator {
13+
return &urlValidator{
14+
whitelistedEndpoints: whitelistedEndpoints,
15+
allowLocalhost: allowLocalhost,
16+
allowPrivateIPs: true, // Test mode allows external URLs by default
17+
}
18+
}
19+
20+
// InitForTest creates a component instance for testing with configurable validation
21+
// whitelist: URLs to allow (nil/empty = allow all external URLs)
22+
// allowLocalhost: whether to allow localhost/127.x.x.x URLs
23+
func InitForTest(bc base.Component, whitelist []string, allowLocalhost bool) *component {
24+
c := &component{
25+
Component: bc,
26+
urlValidator: NewTestURLValidator(whitelist, allowLocalhost),
27+
}
28+
err := c.LoadDefinition(definitionYAML, setupYAML, tasksYAML, nil, nil)
29+
if err != nil {
30+
panic(err)
31+
}
32+
return c
33+
}
34+
1135
// TestURLValidation tests the URL validation logic comprehensively
1236
func TestURLValidation(t *testing.T) {
1337
c := qt.New(t)
@@ -89,14 +113,17 @@ func TestURLValidation(t *testing.T) {
89113
// The actual result depends on config values and DNS resolution
90114
if tc.expectBlock {
91115
c.Assert(err, qt.IsNotNil, qt.Commentf("Should be blocked: %s (%s)", tc.url, tc.reason))
92-
} else {
93-
// These might fail due to DNS in test environment, but that's expected
94-
// The important thing is that the whitelist logic is in place
95-
if err != nil {
96-
// If it fails, it should be due to DNS, not whitelist logic
97-
c.Assert(err.Error(), qt.Contains, "lookup", qt.Commentf("If blocked, should be due to DNS lookup, not whitelist logic"))
98-
}
116+
return
99117
}
118+
119+
// These might fail due to DNS in test environment, but that's expected
120+
// The important thing is that the whitelist logic is in place
121+
if err == nil {
122+
return
123+
}
124+
125+
// If it fails, it should be due to DNS, not whitelist logic
126+
c.Assert(err.Error(), qt.Contains, "lookup", qt.Commentf("If blocked, should be due to DNS lookup, not whitelist logic"))
100127
})
101128
}
102129
})

0 commit comments

Comments
 (0)