From 7fe07174d50c86264f19921f51624b3638daf08c Mon Sep 17 00:00:00 2001 From: Horacio Duran Date: Sun, 24 Nov 2024 15:33:24 +0100 Subject: [PATCH 1/3] This adds a test to bug 508 It fails against master before merging the patch on https://github.com/magefile/mage/pull/509#pullrequestreview-2456725594 --- mage/testdata/bug508/ambiguousimports.go | 20 +++++++ parse/parse_test.go | 74 ++++++++++++++++++++++++ 2 files changed, 94 insertions(+) create mode 100644 mage/testdata/bug508/ambiguousimports.go diff --git a/mage/testdata/bug508/ambiguousimports.go b/mage/testdata/bug508/ambiguousimports.go new file mode 100644 index 00000000..416a179b --- /dev/null +++ b/mage/testdata/bug508/ambiguousimports.go @@ -0,0 +1,20 @@ +package bug508 + +import ( + "fmt" + + "github.com/magefile/mage/mg" +) + +// All code in this package belongs to @na4ma4 in GitHub https://github.com/na4ma4/magefile-test-import +// reproduced here for ease of testing regression on bug 508 + +type Docker mg.Namespace + +func (Docker) Test() { + fmt.Println("docker") +} + +func Test() { + fmt.Println("test") +} diff --git a/parse/parse_test.go b/parse/parse_test.go index fafcf9f1..3a5cf333 100644 --- a/parse/parse_test.go +++ b/parse/parse_test.go @@ -1,6 +1,9 @@ package parse import ( + "go/ast" + "go/parser" + "go/token" "log" "os" "reflect" @@ -112,3 +115,74 @@ func TestGetImportSelf(t *testing.T) { t.Fatalf("expected package importself, got %v", imp.Info.AstPkg.Name) } } + +func TestGetFunction(t *testing.T) { + // This test issue #508 :Bug: using Default with imports selects first matching func by name + // Credit on the test case code goes to @na4ma4 + // Setup the AST for the provided code + src := ` +package magefile + +import ( + "github.com/magefile/mage/mage/testdata/bug508" +) + +var Default = bug508.Test +` + fset := token.NewFileSet() + node, err := parser.ParseFile(fset, "magefile.go", src, parser.AllErrors) + if err != nil { + t.Fatalf("failed to parse source: %v", err) + } + + bug508, err := getImport("go", "github.com/magefile/mage/mage/testdata/bug508", "") + if err != nil { + t.Fatalf("failed to get import: %v", err) + } + // Create PkgInfo + pi := &PkgInfo{ + AstPkg: &ast.Package{ + Name: "magefile", + Files: map[string]*ast.File{"magefile.go": node}, + }, + Imports: Imports{ + bug508, + }, + Funcs: Functions{ + &Function{Package: "bug508", Name: "Test", Receiver: "Docker"}, + &Function{Package: "bug508", Name: "Test"}, + }, + } + + // Create the ast.Expr for the Default variable + var expr ast.Expr + for _, decl := range node.Decls { + if genDecl, ok := decl.(*ast.GenDecl); ok && genDecl.Tok == token.VAR { + for _, spec := range genDecl.Specs { + if valueSpec, ok := spec.(*ast.ValueSpec); ok { + if valueSpec.Names[0].Name == "Default" { + expr = valueSpec.Values[0] + } + } + } + } + } + + // Call getFunction + fn, err := getFunction(expr, pi) + if err != nil { + t.Fatalf("getFunction() error = %v", err) + } + + // Verify the result + expected := &Function{Name: "Test"} + if fn.Name != expected.Name { + t.Errorf("expected function name %q, got %q", expected.Name, fn.Name) + } + if fn.Receiver != "" { + t.Errorf("expected receiver to be empty, got %q", fn.Receiver) + } + + t.Logf("fn: %#v", fn) + t.FailNow() +} From ef0f07011185ea9fbd896387985f7b8427899f59 Mon Sep 17 00:00:00 2001 From: Horacio Duran Date: Sun, 24 Nov 2024 17:18:06 +0100 Subject: [PATCH 2/3] Remove "FailNow" fuse to alwais fail --- parse/parse_test.go | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/parse/parse_test.go b/parse/parse_test.go index 3a5cf333..cddf537a 100644 --- a/parse/parse_test.go +++ b/parse/parse_test.go @@ -140,6 +140,7 @@ var Default = bug508.Test t.Fatalf("failed to get import: %v", err) } // Create PkgInfo + pi := &PkgInfo{ AstPkg: &ast.Package{ Name: "magefile", @@ -148,13 +149,8 @@ var Default = bug508.Test Imports: Imports{ bug508, }, - Funcs: Functions{ - &Function{Package: "bug508", Name: "Test", Receiver: "Docker"}, - &Function{Package: "bug508", Name: "Test"}, - }, } - // Create the ast.Expr for the Default variable var expr ast.Expr for _, decl := range node.Decls { if genDecl, ok := decl.(*ast.GenDecl); ok && genDecl.Tok == token.VAR { @@ -162,6 +158,7 @@ var Default = bug508.Test if valueSpec, ok := spec.(*ast.ValueSpec); ok { if valueSpec.Names[0].Name == "Default" { expr = valueSpec.Values[0] + break } } } @@ -182,7 +179,4 @@ var Default = bug508.Test if fn.Receiver != "" { t.Errorf("expected receiver to be empty, got %q", fn.Receiver) } - - t.Logf("fn: %#v", fn) - t.FailNow() } From c70362033a8a2d7711f1423ce6500afbcb88aade Mon Sep 17 00:00:00 2001 From: Horacio Duran Date: Sun, 24 Nov 2024 19:01:39 +0100 Subject: [PATCH 3/3] Make the test more accessible This way the test better reflects a regular run of mage. --- mage/main_test.go | 22 ++++++ .../bug508/{ => deps}/ambiguousimports.go | 2 +- mage/testdata/bug508/magefile.go | 11 +++ parse/parse_test.go | 68 ------------------- 4 files changed, 34 insertions(+), 69 deletions(-) rename mage/testdata/bug508/{ => deps}/ambiguousimports.go (95%) create mode 100644 mage/testdata/bug508/magefile.go diff --git a/mage/main_test.go b/mage/main_test.go index 07e598e5..8b21fb4b 100644 --- a/mage/main_test.go +++ b/mage/main_test.go @@ -1877,6 +1877,28 @@ func TestWrongDependency(t *testing.T) { } } +// Regression tests, add tests to ensure we do not regress on known issues. + +// TestBug508 is a regression test for: Bug: using Default with imports selects first matching func by name +func TestBug508(t *testing.T) { + stdout := &bytes.Buffer{} + stderr := &bytes.Buffer{} + inv := Invocation{ + Dir: "./testdata/bug508", + Stderr: stderr, + Stdout: stdout, + } + code := Invoke(inv) + if code != 0 { + t.Log(stderr.String()) + t.Fatalf("expected 0, but got %v", code) + } + expected := "test\n" + if stdout.String() != expected { + t.Fatalf("expected %q, but got %q", expected, stdout.String()) + } +} + // / This code liberally borrowed from https://github.com/rsc/goversion/blob/master/version/exe.go type ( diff --git a/mage/testdata/bug508/ambiguousimports.go b/mage/testdata/bug508/deps/ambiguousimports.go similarity index 95% rename from mage/testdata/bug508/ambiguousimports.go rename to mage/testdata/bug508/deps/ambiguousimports.go index 416a179b..631e2602 100644 --- a/mage/testdata/bug508/ambiguousimports.go +++ b/mage/testdata/bug508/deps/ambiguousimports.go @@ -1,4 +1,4 @@ -package bug508 +package deps import ( "fmt" diff --git a/mage/testdata/bug508/magefile.go b/mage/testdata/bug508/magefile.go new file mode 100644 index 00000000..65870a71 --- /dev/null +++ b/mage/testdata/bug508/magefile.go @@ -0,0 +1,11 @@ +//go:build mage +// +build mage + +package main + +import ( + //mage:import + "github.com/magefile/mage/mage/testdata/bug508/deps" +) + +var Default = deps.Test diff --git a/parse/parse_test.go b/parse/parse_test.go index cddf537a..fafcf9f1 100644 --- a/parse/parse_test.go +++ b/parse/parse_test.go @@ -1,9 +1,6 @@ package parse import ( - "go/ast" - "go/parser" - "go/token" "log" "os" "reflect" @@ -115,68 +112,3 @@ func TestGetImportSelf(t *testing.T) { t.Fatalf("expected package importself, got %v", imp.Info.AstPkg.Name) } } - -func TestGetFunction(t *testing.T) { - // This test issue #508 :Bug: using Default with imports selects first matching func by name - // Credit on the test case code goes to @na4ma4 - // Setup the AST for the provided code - src := ` -package magefile - -import ( - "github.com/magefile/mage/mage/testdata/bug508" -) - -var Default = bug508.Test -` - fset := token.NewFileSet() - node, err := parser.ParseFile(fset, "magefile.go", src, parser.AllErrors) - if err != nil { - t.Fatalf("failed to parse source: %v", err) - } - - bug508, err := getImport("go", "github.com/magefile/mage/mage/testdata/bug508", "") - if err != nil { - t.Fatalf("failed to get import: %v", err) - } - // Create PkgInfo - - pi := &PkgInfo{ - AstPkg: &ast.Package{ - Name: "magefile", - Files: map[string]*ast.File{"magefile.go": node}, - }, - Imports: Imports{ - bug508, - }, - } - - var expr ast.Expr - for _, decl := range node.Decls { - if genDecl, ok := decl.(*ast.GenDecl); ok && genDecl.Tok == token.VAR { - for _, spec := range genDecl.Specs { - if valueSpec, ok := spec.(*ast.ValueSpec); ok { - if valueSpec.Names[0].Name == "Default" { - expr = valueSpec.Values[0] - break - } - } - } - } - } - - // Call getFunction - fn, err := getFunction(expr, pi) - if err != nil { - t.Fatalf("getFunction() error = %v", err) - } - - // Verify the result - expected := &Function{Name: "Test"} - if fn.Name != expected.Name { - t.Errorf("expected function name %q, got %q", expected.Name, fn.Name) - } - if fn.Receiver != "" { - t.Errorf("expected receiver to be empty, got %q", fn.Receiver) - } -}