From 117a9b82958fbee49be96c289790a83a303df883 Mon Sep 17 00:00:00 2001 From: Alexandre Daubois Date: Fri, 24 Apr 2026 11:02:36 +0200 Subject: [PATCH] test(extgen): tighten parser tests by asserting warnings and dropping misleading conditionals --- internal/extgen/classparser_test.go | 49 ++++---- internal/extgen/constparser_test.go | 169 +++++++++++++--------------- internal/extgen/funcparser_test.go | 161 +++++++++++++++++--------- internal/extgen/helpers_test.go | 32 ++++++ internal/extgen/integration_test.go | 6 +- 5 files changed, 246 insertions(+), 171 deletions(-) create mode 100644 internal/extgen/helpers_test.go diff --git a/internal/extgen/classparser_test.go b/internal/extgen/classparser_test.go index 11454c48e2..f231b2722b 100644 --- a/internal/extgen/classparser_test.go +++ b/internal/extgen/classparser_test.go @@ -11,9 +11,10 @@ import ( func TestClassParser(t *testing.T) { tests := []struct { - name string - input string - expected int + name string + input string + expect int + assert func(t *testing.T, classes []phpClass) }{ { name: "single class", @@ -24,7 +25,13 @@ type UserStruct struct { name string Age int }`, - expected: 1, + expect: 1, + assert: func(t *testing.T, classes []phpClass) { + c := classes[0] + assert.Equal(t, "User", c.Name) + assert.Equal(t, "UserStruct", c.GoStruct) + assert.Len(t, c.Properties, 2) + }, }, { name: "multiple classes", @@ -41,7 +48,7 @@ type ProductStruct struct { Title string Price float64 }`, - expected: 2, + expect: 2, }, { name: "no php classes", @@ -50,7 +57,7 @@ type ProductStruct struct { type RegularStruct struct { Data string }`, - expected: 0, + expect: 0, }, { name: "class with nullable fields", @@ -62,7 +69,14 @@ type OptionalStruct struct { Optional *string Count *int }`, - expected: 1, + expect: 1, + assert: func(t *testing.T, classes []phpClass) { + require.Len(t, classes[0].Properties, 3) + props := classes[0].Properties + assert.False(t, props[0].IsNullable, "Required field should not be nullable") + assert.True(t, props[1].IsNullable, "Optional field should be nullable") + assert.True(t, props[2].IsNullable, "Count field should be nullable") + }, }, { name: "class with methods", @@ -83,7 +97,7 @@ func GetUserName(u UserStruct) string { func SetUserAge(u *UserStruct, age int) { u.Age = age }`, - expected: 1, + expect: 1, }, } @@ -96,23 +110,10 @@ func SetUserAge(u *UserStruct, age int) { parser := classParser{} classes, err := parser.parse(fileName) require.NoError(t, err) + require.Len(t, classes, tt.expect) - assert.Len(t, classes, tt.expected, "parse() got wrong number of classes") - - if tt.name == "single class" && len(classes) > 0 { - class := classes[0] - assert.Equal(t, "User", class.Name, "Expected class name 'User'") - assert.Equal(t, "UserStruct", class.GoStruct, "Expected Go struct 'UserStruct'") - assert.Len(t, class.Properties, 2, "Expected 2 properties") - } - - if tt.name == "class with nullable fields" && len(classes) > 0 { - class := classes[0] - if len(class.Properties) >= 3 { - assert.False(t, class.Properties[0].IsNullable, "Required field should not be nullable") - assert.True(t, class.Properties[1].IsNullable, "Optional field should be nullable") - assert.True(t, class.Properties[2].IsNullable, "Count field should be nullable") - } + if tt.assert != nil { + tt.assert(t, classes) } }) } diff --git a/internal/extgen/constparser_test.go b/internal/extgen/constparser_test.go index 7c5ce89f27..cf73ac7838 100644 --- a/internal/extgen/constparser_test.go +++ b/internal/extgen/constparser_test.go @@ -11,9 +11,10 @@ import ( func TestConstantParser(t *testing.T) { tests := []struct { - name string - input string - expected int + name string + input string + expect int + assert func(t *testing.T, constants []phpConstant) }{ { name: "single constant", @@ -21,7 +22,14 @@ func TestConstantParser(t *testing.T) { //export_php:const const MyConstant = "test_value"`, - expected: 1, + expect: 1, + assert: func(t *testing.T, cs []phpConstant) { + c := cs[0] + assert.Equal(t, "MyConstant", c.Name) + assert.Equal(t, `"test_value"`, c.Value) + assert.Equal(t, phpString, c.PhpType) + assert.False(t, c.IsIota) + }, }, { name: "multiple constants", @@ -35,7 +43,17 @@ const SecondConstant = 42 //export_php:const const ThirdConstant = true`, - expected: 3, + expect: 3, + assert: func(t *testing.T, cs []phpConstant) { + names := []string{"FirstConstant", "SecondConstant", "ThirdConstant"} + values := []string{`"first"`, "42", "true"} + types := []phpType{phpString, phpInt, phpBool} + for i, c := range cs { + assert.Equal(t, names[i], c.Name) + assert.Equal(t, values[i], c.Value) + assert.Equal(t, types[i], c.PhpType) + } + }, }, { name: "iota constant", @@ -43,7 +61,13 @@ const ThirdConstant = true`, //export_php:const const IotaConstant = iota`, - expected: 1, + expect: 1, + assert: func(t *testing.T, cs []phpConstant) { + c := cs[0] + assert.Equal(t, "IotaConstant", c.Name) + assert.True(t, c.IsIota) + assert.Equal(t, "0", c.Value) + }, }, { name: "mixed constants and iota", @@ -57,7 +81,7 @@ const IotaConst = iota //export_php:const const IntConst = 123`, - expected: 3, + expect: 3, }, { name: "no php constants", @@ -68,7 +92,7 @@ const RegularConstant = "not exported" func someFunction() { // Just regular code }`, - expected: 0, + expect: 0, }, { name: "constant with complex value", @@ -76,7 +100,7 @@ func someFunction() { //export_php:const const ComplexConstant = "string with spaces and symbols !@#$%"`, - expected: 1, + expect: 1, }, { name: "directive without constant", @@ -84,7 +108,7 @@ const ComplexConstant = "string with spaces and symbols !@#$%"`, //export_php:const var notAConstant = "this is a variable"`, - expected: 0, + expect: 0, }, { name: "mixed export and non-export constants", @@ -99,7 +123,7 @@ const AnotherRegular = 456 //export_php:const const AnotherExported = 789`, - expected: 2, + expect: 2, }, { name: "numeric constants", @@ -113,7 +137,7 @@ const FloatConstant = 3.14 //export_php:const const HexConstant = 0xFF`, - expected: 3, + expect: 3, }, { name: "boolean constants", @@ -124,7 +148,7 @@ const TrueConstant = true //export_php:const const FalseConstant = false`, - expected: 2, + expect: 2, }, } @@ -136,35 +160,11 @@ const FalseConstant = false`, parser := &ConstantParser{} constants, err := parser.parse(tmpFile) - assert.NoError(t, err, "parse() error") - - assert.Len(t, constants, tt.expected, "parse() got wrong number of constants") - - if tt.name == "single constant" && len(constants) > 0 { - c := constants[0] - assert.Equal(t, "MyConstant", c.Name, "Expected constant name 'MyConstant'") - assert.Equal(t, `"test_value"`, c.Value, `Expected constant value '"test_value"'`) - assert.Equal(t, phpString, c.PhpType, "Expected constant type 'string'") - assert.False(t, c.IsIota, "Expected isIota to be false for string constant") - } + require.NoError(t, err) + require.Len(t, constants, tt.expect) - if tt.name == "iota constant" && len(constants) > 0 { - c := constants[0] - assert.Equal(t, "IotaConstant", c.Name, "Expected constant name 'IotaConstant'") - assert.True(t, c.IsIota, "Expected isIota to be true") - assert.Equal(t, "0", c.Value, "Expected iota constant value to be '0'") - } - - if tt.name == "multiple constants" && len(constants) == 3 { - expectedNames := []string{"FirstConstant", "SecondConstant", "ThirdConstant"} - expectedValues := []string{`"first"`, "42", "true"} - expectedTypes := []phpType{phpString, phpInt, phpBool} - - for i, c := range constants { - assert.Equal(t, expectedNames[i], c.Name, "Expected constant name '%s'", expectedNames[i]) - assert.Equal(t, expectedValues[i], c.Value, "Expected constant value '%s'", expectedValues[i]) - assert.Equal(t, expectedTypes[i], c.PhpType, "Expected constant type '%s'", expectedTypes[i]) - } + if tt.assert != nil { + tt.assert(t, constants) } }) } @@ -172,9 +172,8 @@ const FalseConstant = false`, func TestConstantParserErrors(t *testing.T) { tests := []struct { - name string - input string - expectError bool + name string + input string }{ { name: "invalid constant declaration", @@ -182,7 +181,6 @@ func TestConstantParserErrors(t *testing.T) { //export_php:const const = "missing name"`, - expectError: true, }, { name: "malformed constant", @@ -190,7 +188,6 @@ const = "missing name"`, //export_php:const const InvalidSyntax`, - expectError: true, }, } @@ -202,15 +199,7 @@ const InvalidSyntax`, parser := &ConstantParser{} _, err := parser.parse(tmpFile) - require.NotNil(t, err) - - if tt.expectError { - assert.Error(t, err, "Expected error but got none") - - return - } - - assert.NoError(t, err) + assert.Error(t, err, "Expected error but got none") }) } } @@ -449,9 +438,10 @@ func TestConstantParserTypeDetection(t *testing.T) { func TestConstantParserClassConstants(t *testing.T) { tests := []struct { - name string - input string - expected int + name string + input string + expect int + assert func(t *testing.T, constants []phpConstant) }{ { name: "single class constant", @@ -459,7 +449,14 @@ func TestConstantParserClassConstants(t *testing.T) { //export_php:classconst MyClass const STATUS_ACTIVE = 1`, - expected: 1, + expect: 1, + assert: func(t *testing.T, cs []phpConstant) { + c := cs[0] + assert.Equal(t, "STATUS_ACTIVE", c.Name) + assert.Equal(t, "MyClass", c.ClassName) + assert.Equal(t, "1", c.Value) + assert.Equal(t, phpInt, c.PhpType) + }, }, { name: "multiple class constants", @@ -473,7 +470,17 @@ const STATUS_INACTIVE = "inactive" //export_php:classconst Order const STATE_PENDING = 0`, - expected: 3, + expect: 3, + assert: func(t *testing.T, cs []phpConstant) { + classes := []string{"User", "User", "Order"} + names := []string{"STATUS_ACTIVE", "STATUS_INACTIVE", "STATE_PENDING"} + values := []string{`"active"`, `"inactive"`, "0"} + for i, c := range cs { + assert.Equal(t, classes[i], c.ClassName) + assert.Equal(t, names[i], c.Name) + assert.Equal(t, values[i], c.Value) + } + }, }, { name: "mixed global and class constants", @@ -487,7 +494,12 @@ const CLASS_CONST = 42 //export_php:const const ANOTHER_GLOBAL = true`, - expected: 3, + expect: 3, + assert: func(t *testing.T, cs []phpConstant) { + assert.Empty(t, cs[0].ClassName, "First constant should be global") + assert.Equal(t, "MyClass", cs[1].ClassName) + assert.Empty(t, cs[2].ClassName, "Third constant should be global") + }, }, { name: "class constant with iota", @@ -498,7 +510,7 @@ const FIRST = iota //export_php:classconst Status const SECOND = iota`, - expected: 2, + expect: 2, }, { name: "invalid class constant directive", @@ -506,7 +518,7 @@ const SECOND = iota`, //export_php:classconst const INVALID = "missing class name"`, - expected: 0, + expect: 0, }, } @@ -518,34 +530,11 @@ const INVALID = "missing class name"`, parser := &ConstantParser{} constants, err := parser.parse(tmpFile) - assert.NoError(t, err, "parse() error") - - assert.Len(t, constants, tt.expected, "parse() got wrong number of constants") - - if tt.name == "single class constant" && len(constants) > 0 { - c := constants[0] - assert.Equal(t, "STATUS_ACTIVE", c.Name, "Expected constant name 'STATUS_ACTIVE'") - assert.Equal(t, "MyClass", c.ClassName, "Expected class name 'MyClass'") - assert.Equal(t, "1", c.Value, "Expected constant value '1'") - assert.Equal(t, phpInt, c.PhpType, "Expected constant type 'int'") - } - - if tt.name == "multiple class constants" && len(constants) == 3 { - expectedClasses := []string{"User", "User", "Order"} - expectedNames := []string{"STATUS_ACTIVE", "STATUS_INACTIVE", "STATE_PENDING"} - expectedValues := []string{`"active"`, `"inactive"`, "0"} - - for i, c := range constants { - assert.Equal(t, expectedClasses[i], c.ClassName, "Expected class name '%s'", expectedClasses[i]) - assert.Equal(t, expectedNames[i], c.Name, "Expected constant name '%s'", expectedNames[i]) - assert.Equal(t, expectedValues[i], c.Value, "Expected constant value '%s'", expectedValues[i]) - } - } + require.NoError(t, err) + require.Len(t, constants, tt.expect) - if tt.name == "mixed global and class constants" && len(constants) == 3 { - assert.Empty(t, constants[0].ClassName, "First constant should be global") - assert.Equal(t, "MyClass", constants[1].ClassName, "Second constant should belong to MyClass") - assert.Empty(t, constants[2].ClassName, "Third constant should be global") + if tt.assert != nil { + tt.assert(t, constants) } }) } diff --git a/internal/extgen/funcparser_test.go b/internal/extgen/funcparser_test.go index 282d23b4f4..71917f2294 100644 --- a/internal/extgen/funcparser_test.go +++ b/internal/extgen/funcparser_test.go @@ -11,9 +11,10 @@ import ( func TestFunctionParser(t *testing.T) { tests := []struct { - name string - input string - expected int + name string + input string + expect int + assert func(t *testing.T, functions []phpFunction) }{ { name: "single function", @@ -23,7 +24,14 @@ func TestFunctionParser(t *testing.T) { func testFunc(name *C.zend_string) unsafe.Pointer { return String("Hello " + CStringToGoString(name)) }`, - expected: 1, + expect: 1, + assert: func(t *testing.T, fns []phpFunction) { + fn := fns[0] + assert.Equal(t, "testFunc", fn.Name) + assert.Equal(t, phpString, fn.ReturnType) + require.Len(t, fn.Params, 1) + assert.Equal(t, "name", fn.Params[0].Name) + }, }, { name: "multiple functions", @@ -34,11 +42,11 @@ func func1(a int64) int64 { return a * 2 } -//export_php:function func2(string $b): string +//export_php:function func2(string $b): string func func2(b *C.zend_string) unsafe.Pointer { return String("processed: " + CStringToGoString(b)) }`, - expected: 2, + expect: 2, }, { name: "no php functions", @@ -47,7 +55,7 @@ func func2(b *C.zend_string) unsafe.Pointer { func regularFunc() { // Just a regular Go function }`, - expected: 0, + expect: 0, }, { name: "mixed functions", @@ -66,7 +74,7 @@ func internalFunc() { func anotherPhpFunc(num int64) int64 { return num * 10 }`, - expected: 2, + expect: 2, }, { name: "wrong args syntax", @@ -76,7 +84,7 @@ func anotherPhpFunc(num int64) int64 { func phpFunc(data *C.zend_string) unsafe.Pointer { return String("PHP: " + CStringToGoString(data)) }`, - expected: 0, + expect: 0, }, { name: "decoupled function names", @@ -91,7 +99,11 @@ func myGoFunction(name *C.zend_string) unsafe.Pointer { func someOtherGoName(num int64) int64 { return num * 5 }`, - expected: 2, + expect: 2, + assert: func(t *testing.T, fns []phpFunction) { + assert.Equal(t, "my_php_function", fns[0].Name) + assert.Equal(t, "another_php_func", fns[1].Name) + }, }, } @@ -104,23 +116,10 @@ func someOtherGoName(num int64) int64 { parser := &FuncParser{} functions, err := parser.parse(fileName) require.NoError(t, err) - assert.Len(t, functions, tt.expected, "parse() got wrong number of functions") - - if tt.name == "single function" && len(functions) > 0 { - fn := functions[0] - assert.Equal(t, "testFunc", fn.Name, "Expected function name 'testFunc'") - assert.Equal(t, phpString, fn.ReturnType, "Expected return type 'string'") - assert.Len(t, fn.Params, 1, "Expected 1 parameter") - if len(fn.Params) > 0 { - assert.Equal(t, "name", fn.Params[0].Name, "Expected parameter name 'name'") - } - } + require.Len(t, functions, tt.expect) - if tt.name == "decoupled function names" && len(functions) >= 2 { - fn1 := functions[0] - assert.Equal(t, "my_php_function", fn1.Name, "Expected PHP function name 'my_php_function'") - fn2 := functions[1] - assert.Equal(t, "another_php_func", fn2.Name, "Expected PHP function name 'another_php_func'") + if tt.assert != nil { + tt.assert(t, functions) } }) } @@ -128,13 +127,14 @@ func someOtherGoName(num int64) int64 { func TestSignatureParsing(t *testing.T) { tests := []struct { - name string - signature string - expectError bool - funcName string - paramCount int - returnType phpType - nullable bool + name string + signature string + expectError bool + funcName string + paramCount int + returnType phpType + nullable bool + paramsNullable []bool }{ { name: "simple function", @@ -169,12 +169,13 @@ func TestSignatureParsing(t *testing.T) { nullable: false, }, { - name: "nullable parameters", - signature: "process(?string data, ?int count): bool", - funcName: "process", - paramCount: 2, - returnType: phpBool, - nullable: false, + name: "nullable parameters", + signature: "process(?string data, ?int count): bool", + funcName: "process", + paramCount: 2, + returnType: phpBool, + nullable: false, + paramsNullable: []bool{true, true}, }, { name: "invalid signature", @@ -198,17 +199,14 @@ func TestSignatureParsing(t *testing.T) { return } - assert.NoError(t, err, "parseSignature() unexpected error") - assert.Equal(t, tt.funcName, fn.Name, "parseSignature() name mismatch") - assert.Len(t, fn.Params, tt.paramCount, "parseSignature() param count mismatch") - assert.Equal(t, tt.returnType, fn.ReturnType, "parseSignature() return type mismatch") - assert.Equal(t, tt.nullable, fn.IsReturnNullable, "parseSignature() nullable mismatch") - - if tt.name == "nullable parameters" { - if len(fn.Params) >= 2 { - assert.True(t, fn.Params[0].IsNullable, "First parameter should be nullable") - assert.True(t, fn.Params[1].IsNullable, "Second parameter should be nullable") - } + require.NoError(t, err) + assert.Equal(t, tt.funcName, fn.Name) + require.Len(t, fn.Params, tt.paramCount) + assert.Equal(t, tt.returnType, fn.ReturnType) + assert.Equal(t, tt.nullable, fn.IsReturnNullable) + + for i, nullable := range tt.paramsNullable { + assert.Equal(t, nullable, fn.Params[i].IsNullable, "param %d nullable mismatch", i) } }) } @@ -294,6 +292,41 @@ func TestParameterParsing(t *testing.T) { } } +func TestFunctionParserRejectsInvalidSignature(t *testing.T) { + // signature without parentheses cannot be parsed; the function must be silently dropped. + input := `package main + +//export_php:function brokenSignatureNoParens +func brokenSignatureNoParens() {} +` + tmpDir := t.TempDir() + fileName := filepath.Join(tmpDir, "test.go") + require.NoError(t, os.WriteFile(fileName, []byte(input), 0644)) + + parser := &FuncParser{} + functions, err := parser.parse(fileName) + require.NoError(t, err) + assert.Empty(t, functions, "function with invalid signature should be dropped") +} + +func TestFunctionParserOrphanDirectiveIsError(t *testing.T) { + // directive not followed by a func must return an error, otherwise the author + // silently loses an export. + input := `package main + +//export_php:function orphan(string $x): string +var notAFunction = 42 +` + tmpDir := t.TempDir() + fileName := filepath.Join(tmpDir, "test.go") + require.NoError(t, os.WriteFile(fileName, []byte(input), 0644)) + + parser := &FuncParser{} + _, err := parser.parse(fileName) + require.Error(t, err, "orphan directive must surface as an error") + assert.Contains(t, err.Error(), "not followed by a function declaration") +} + func TestFunctionParserUnsupportedTypes(t *testing.T) { tests := []struct { name string @@ -386,11 +419,21 @@ func voidFunc(message *C.zend_string) { tmpFile := filepath.Join(tmpDir, tt.name+".go") require.NoError(t, os.WriteFile(tmpFile, []byte(tt.input), 0644)) - parser := &FuncParser{} - functions, err := parser.parse(tmpFile) + var functions []phpFunction + var err error + stdout := captureStdout(t, func() { + parser := &FuncParser{} + functions, err = parser.parse(tmpFile) + }) require.NoError(t, err) assert.Len(t, functions, tt.expected, "parse() got wrong number of functions") + + if tt.hasWarning { + assert.Contains(t, stdout, "Warning:", "expected a warning to be emitted") + } else { + assert.NotContains(t, stdout, "Warning:", "no warning expected") + } }) } } @@ -476,11 +519,21 @@ func validFloat(value float64) float64 { fileName := filepath.Join(tmpDir, tt.name+".go") require.NoError(t, os.WriteFile(fileName, []byte(tt.input), 0644)) - parser := &FuncParser{} - functions, err := parser.parse(fileName) + var functions []phpFunction + var err error + stdout := captureStdout(t, func() { + parser := &FuncParser{} + functions, err = parser.parse(fileName) + }) require.NoError(t, err) assert.Len(t, functions, tt.expected, "parse() got wrong number of functions") + + if tt.hasWarning { + assert.Contains(t, stdout, "Warning:", "expected a warning to be emitted") + } else { + assert.NotContains(t, stdout, "Warning:", "no warning expected") + } }) } } diff --git a/internal/extgen/helpers_test.go b/internal/extgen/helpers_test.go new file mode 100644 index 0000000000..3addfe28ef --- /dev/null +++ b/internal/extgen/helpers_test.go @@ -0,0 +1,32 @@ +package extgen + +import ( + "io" + "os" + "testing" +) + +// captureStdout redirects os.Stdout, runs fn, and returns what was written. +func captureStdout(t *testing.T, fn func()) string { + t.Helper() + original := os.Stdout + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("pipe: %v", err) + } + os.Stdout = w + + done := make(chan string, 1) + go func() { + b, _ := io.ReadAll(r) + done <- string(b) + }() + + fn() + + if err := w.Close(); err != nil { + t.Fatalf("close pipe writer: %v", err) + } + os.Stdout = original + return <-done +} diff --git a/internal/extgen/integration_test.go b/internal/extgen/integration_test.go index c8cdd90c9a..2381b0d6f4 100644 --- a/internal/extgen/integration_test.go +++ b/internal/extgen/integration_test.go @@ -38,18 +38,18 @@ func setupTest(t *testing.T) *IntegrationTestSuite { } if _, err := os.Stat(suite.genStubScript); os.IsNotExist(err) { - t.Error("GEN_STUB_SCRIPT not found. Integration tests require PHP sources. Set GEN_STUB_SCRIPT environment variable.") + t.Skipf("GEN_STUB_SCRIPT not found at %q. Integration tests require PHP sources. Set GEN_STUB_SCRIPT environment variable to skip this message.", suite.genStubScript) } xcaddyPath, err := exec.LookPath("xcaddy") if err != nil { - t.Error("xcaddy not found in PATH. Integration tests require xcaddy to build FrankenPHP.") + t.Skip("xcaddy not found in PATH. Integration tests require xcaddy to build FrankenPHP.") } suite.xcaddyPath = xcaddyPath phpConfigPath, err := exec.LookPath("php-config") if err != nil { - t.Error("php-config not found in PATH. Integration tests require PHP development headers.") + t.Skip("php-config not found in PATH. Integration tests require PHP development headers.") } suite.phpConfigPath = phpConfigPath