From 7b2b823d66f6504922e5c58def61ca9571d9c8fe Mon Sep 17 00:00:00 2001 From: Noble Mittal Date: Tue, 13 Feb 2024 02:40:49 +0530 Subject: [PATCH 1/5] Initialize the flag test Signed-off-by: Noble Mittal --- go/internal/flag/flag_test.go | 45 +++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) create mode 100644 go/internal/flag/flag_test.go diff --git a/go/internal/flag/flag_test.go b/go/internal/flag/flag_test.go new file mode 100644 index 00000000000..60076003bd1 --- /dev/null +++ b/go/internal/flag/flag_test.go @@ -0,0 +1,45 @@ +/* +Copyright 2024 The Vitess Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package flag + +import ( + "flag" + "fmt" + "testing" + + "github.com/spf13/pflag" +) + +func TestPreventGlogVFlagFromClobberingVersionFlagShorthand(t *testing.T) { + var v bool + + flag.BoolVar(&v, "v", true, "test flag description") + + testFlagSet := pflag.NewFlagSet("testFlagSet", pflag.ExitOnError) + PreventGlogVFlagFromClobberingVersionFlagShorthand(testFlagSet) + + fmt.Println(testFlagSet.Lookup("v")) + // f := pflag.Lookup("v") + // assert.Equal(t, f.Shorthand, "") + + // testFlagSet.BoolVar(&vtest, "vtest", true, "test flag description") + // PreventGlogVFlagFromClobberingVersionFlagShorthand(testFlagSet) +} + +func TestParse(t *testing.T) { + +} From a8f82c21fb428bcc863c49ca234ff5016e2373b1 Mon Sep 17 00:00:00 2001 From: Noble Mittal Date: Tue, 13 Feb 2024 22:36:38 +0530 Subject: [PATCH 2/5] Add required tests for internal/flag Signed-off-by: Noble Mittal --- go/internal/flag/flag_test.go | 281 +++++++++++++++++++++++++++++++-- go/internal/flag/usage_test.go | 134 ++++++++++++++++ 2 files changed, 406 insertions(+), 9 deletions(-) create mode 100644 go/internal/flag/usage_test.go diff --git a/go/internal/flag/flag_test.go b/go/internal/flag/flag_test.go index 60076003bd1..73e777cac21 100644 --- a/go/internal/flag/flag_test.go +++ b/go/internal/flag/flag_test.go @@ -17,29 +17,292 @@ limitations under the License. package flag import ( - "flag" - "fmt" + goflag "flag" + "os" "testing" "github.com/spf13/pflag" + "github.com/stretchr/testify/assert" ) func TestPreventGlogVFlagFromClobberingVersionFlagShorthand(t *testing.T) { - var v bool + oldCommandLine := goflag.CommandLine + defer func() { + goflag.CommandLine = oldCommandLine + }() - flag.BoolVar(&v, "v", true, "test flag description") + goflag.CommandLine = goflag.NewFlagSet(os.Args[0], goflag.ExitOnError) + + var v, vtest bool + + goflag.BoolVar(&v, "v", true, "") testFlagSet := pflag.NewFlagSet("testFlagSet", pflag.ExitOnError) PreventGlogVFlagFromClobberingVersionFlagShorthand(testFlagSet) - fmt.Println(testFlagSet.Lookup("v")) - // f := pflag.Lookup("v") - // assert.Equal(t, f.Shorthand, "") + f := testFlagSet.Lookup("v") + assert.NotNil(t, f) + assert.Equal(t, "", f.Shorthand) - // testFlagSet.BoolVar(&vtest, "vtest", true, "test flag description") - // PreventGlogVFlagFromClobberingVersionFlagShorthand(testFlagSet) + testFlagSet.BoolVar(&vtest, "vtest", true, "") + PreventGlogVFlagFromClobberingVersionFlagShorthand(testFlagSet) } func TestParse(t *testing.T) { + oldCommandLine := goflag.CommandLine + defer func() { + goflag.CommandLine = oldCommandLine + }() + + var testFlag bool + goflag.CommandLine = goflag.NewFlagSet(os.Args[0], goflag.ExitOnError) + goflag.BoolVar(&testFlag, "testFlag", true, "") + + testFlagSet := pflag.NewFlagSet("testFlagSet", pflag.ExitOnError) + + Parse(testFlagSet) + + f := testFlagSet.ShorthandLookup("h") + assert.NotNil(t, f) + assert.Equal(t, "false", f.DefValue) + + f = testFlagSet.Lookup("help") + assert.NotNil(t, f) + assert.Equal(t, "false", f.DefValue) + + testFlagSet = pflag.NewFlagSet("testFlagSet2", pflag.ExitOnError) + + // If shorthand "h" is already defined, shorthand for "help" should be empty + var h bool + testFlagSet.BoolVarP(&h, "testH", "h", false, "") + + Parse(testFlagSet) + f = testFlagSet.Lookup("help") + assert.NotNil(t, f) + assert.Equal(t, "", f.Shorthand) + + // Check if AddGoFlagSet was called + f = testFlagSet.Lookup("testFlag") + assert.NotNil(t, f) + assert.Equal(t, "true", f.DefValue) +} + +func TestIsFlagProvided(t *testing.T) { + oldPflagCommandLine := pflag.CommandLine + defer func() { + pflag.CommandLine = oldPflagCommandLine + }() + + pflag.CommandLine = pflag.NewFlagSet("testFlagSet", pflag.ExitOnError) + + isProvided := IsFlagProvided("testFlag") + assert.False(t, isProvided) + + var testFlag bool + pflag.BoolVar(&testFlag, "testFlag", false, "") + + // Should return false as testFlag is not set + isProvided = IsFlagProvided("testFlag") + assert.False(t, isProvided) + + pflag.Parse() + _ = pflag.Set("testFlag", "true") + + // Should return true as testFlag is set + isProvided = IsFlagProvided("testFlag") + assert.True(t, isProvided) +} + +func TestFilterTestFlags(t *testing.T) { + oldOsArgs := os.Args + defer func() { + os.Args = oldOsArgs + }() + + os.Args = []string{ + "-test.run", + "TestFilter", + "otherArgs1", + "otherArgs2", + "-test.run=TestFilter", + } + + otherArgs, testFlags := filterTestFlags() + + expectedTestFlags := []string{ + "-test.run", + "TestFilter", + "-test.run=TestFilter", + } + expectedOtherArgs := []string{ + "otherArgs1", + "otherArgs2", + } + + assert.Equal(t, expectedOtherArgs, otherArgs) + assert.Equal(t, expectedTestFlags, testFlags) +} + +func TestParseFlagsForTest(t *testing.T) { + oldOsArgs := os.Args + oldPflagCommandLine := pflag.CommandLine + oldCommandLine := goflag.CommandLine + + defer func() { + os.Args = oldOsArgs + pflag.CommandLine = oldPflagCommandLine + goflag.CommandLine = oldCommandLine + }() + + pflag.CommandLine = pflag.NewFlagSet("testFlagSet", pflag.ExitOnError) + + os.Args = []string{ + "-test.run", + "TestFilter", + "otherArgs1", + "otherArgs2", + "-test.run=TestFilter", + } + + ParseFlagsForTest() + + expectedOsArgs := []string{ + "otherArgs1", + "otherArgs2", + } + + assert.Equal(t, expectedOsArgs, os.Args) + assert.Equal(t, true, pflag.Parsed()) +} + +func TestParsed(t *testing.T) { + oldPflagCommandLine := pflag.CommandLine + oldCommandLine := goflag.CommandLine + + defer func() { + pflag.CommandLine = oldPflagCommandLine + goflag.CommandLine = oldCommandLine + }() + + pflag.CommandLine = pflag.NewFlagSet("testPflagSet", pflag.ExitOnError) + goflag.CommandLine = goflag.NewFlagSet("testGoflagSet", goflag.ExitOnError) + + b := Parsed() + assert.False(t, b) + + pflag.Parse() + b = Parsed() + assert.True(t, b) +} + +func TestLookup(t *testing.T) { + oldPflagCommandLine := pflag.CommandLine + oldCommandLine := goflag.CommandLine + + defer func() { + pflag.CommandLine = oldPflagCommandLine + goflag.CommandLine = oldCommandLine + }() + + pflag.CommandLine = pflag.NewFlagSet("testPflagSet", pflag.ExitOnError) + goflag.CommandLine = goflag.NewFlagSet("testGoflagSet", goflag.ExitOnError) + + var testGoFlag, testPflag, testFlag bool + + goflag.BoolVar(&testGoFlag, "testGoFlag", true, "") + goflag.BoolVar(&testFlag, "t", true, "") + pflag.BoolVar(&testPflag, "testPflag", true, "") + + testCases := []struct { + shorthand string + name string + }{ + { + // If single character flag is passed, the shorthand should be the same + shorthand: "t", + name: "t", + }, + { + shorthand: "", + name: "testGoFlag", + }, + { + shorthand: "", + name: "testPflag", + }, + } + + for _, tt := range testCases { + f := Lookup(tt.name) + + assert.NotNil(t, f) + assert.Equal(t, tt.shorthand, f.Shorthand) + assert.Equal(t, tt.name, f.Name) + } + + f := Lookup("non-existent-flag") + assert.Nil(t, f) +} + +func TestArgs(t *testing.T) { + oldPflagCommandLine := pflag.CommandLine + oldOsArgs := os.Args + + defer func() { + pflag.CommandLine = oldPflagCommandLine + os.Args = oldOsArgs + }() + + pflag.CommandLine = pflag.NewFlagSet("testPflagSet", pflag.ExitOnError) + + os.Args = []string{ + "arg0", + "arg1", + "arg2", + "arg3", + } + + expectedArgs := []string{ + "arg1", + "arg2", + "arg3", + } + + pflag.Parse() + // Should work equivalent to pflag.Args if there's no double dash + args := Args() + assert.Equal(t, expectedArgs, args) + + arg := Arg(2) + assert.Equal(t, "arg3", arg) + + // Should return empty string if the index is greater than len of CommandLine.args + arg = Arg(3) + assert.Equal(t, "", arg) +} + +func TestIsZeroValue(t *testing.T) { + oldCommandLine := goflag.CommandLine + + defer func() { + goflag.CommandLine = oldCommandLine + }() + + var testFlag string + + goflag.StringVar(&testFlag, "testflag", "default", "Description of testflag") + goflag.Parse() + + f := goflag.Lookup("testflag") + + // Test the isZeroValue function + result := isZeroValue(f, "") + + assert.True(t, result) + // Test again with a non-zero value + result = isZeroValue(f, "anyValue") + if result { + t.Errorf("Expected false, got true. The value 'newvalue' should not be considered as zero value.") + } } diff --git a/go/internal/flag/usage_test.go b/go/internal/flag/usage_test.go new file mode 100644 index 00000000000..d88846f8840 --- /dev/null +++ b/go/internal/flag/usage_test.go @@ -0,0 +1,134 @@ +/* +Copyright 2024 The Vitess Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package flag + +import ( + goflag "flag" + "io" + "os" + "strings" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestSetUsage(t *testing.T) { + fs := goflag.NewFlagSet("test", goflag.ExitOnError) + fs.String("testflag", "default", "`test` flag") + + opts := UsageOptions{ + Preface: func(w io.Writer) { + _, _ = w.Write([]byte("test preface")) + }, + Epilogue: func(w io.Writer) { + _, _ = w.Write([]byte("test epilogue")) + }, + FlagFilter: func(f *goflag.Flag) bool { + return f.Value.String() == "default" + }, + } + + SetUsage(fs, opts) + + var builder strings.Builder + fs.SetOutput(&builder) + + _ = fs.Set("testflag", "not default") + fs.Usage() + + output := builder.String() + assert.NotContains(t, output, "test flag") + + // Set the value back to default + _ = fs.Set("testflag", "default") + fs.Usage() + output = builder.String() + + assert.Contains(t, output, "test preface") + assert.Contains(t, output, "--testflag test") + assert.Contains(t, output, "test epilogue") + assert.Contains(t, output, "test flag") +} + +func TestSetUsageWithNilFlagFilterAndPreface(t *testing.T) { + oldOsArgs := os.Args + defer func() { + os.Args = oldOsArgs + }() + + os.Args = []string{"testOsArg"} + fs := goflag.NewFlagSet("test", goflag.ExitOnError) + fs.String("testflag", "default", "`test` flag") + + opts := UsageOptions{ + Epilogue: func(w io.Writer) { + _, _ = w.Write([]byte("test epilogue")) + }, + } + + SetUsage(fs, opts) + + var builder strings.Builder + fs.SetOutput(&builder) + fs.Usage() + output := builder.String() + + assert.Contains(t, output, "Usage of testOsArg:") + assert.Contains(t, output, "--testflag test") + assert.Contains(t, output, "test epilogue") +} + +type testBool struct{} + +func (t testBool) IsBoolFlag() bool { + return true +} + +func (t testBool) String() string { + return "true" +} + +func (t testBool) Set(s string) error { + return nil +} + +func TestSetUsageWithBoolFlag(t *testing.T) { + fs := goflag.NewFlagSet("test2", goflag.ExitOnError) + fs.Var(testBool{}, "t", "`t` flag") + + opts := UsageOptions{ + Preface: func(w io.Writer) { + _, _ = w.Write([]byte("test preface")) + }, + Epilogue: func(w io.Writer) { + _, _ = w.Write([]byte("test epilogue")) + }, + FlagFilter: func(f *goflag.Flag) bool { + return f.Value.String() == "true" + }, + } + + SetUsage(fs, opts) + + var builder strings.Builder + fs.SetOutput(&builder) + fs.Usage() + output := builder.String() + + assert.Contains(t, output, "test preface") + assert.Contains(t, output, "-t\tt flag") +} From 1f246038a275815ca547b103097f96a110aef6c3 Mon Sep 17 00:00:00 2001 From: Noble Mittal Date: Sat, 17 Feb 2024 10:51:15 +0530 Subject: [PATCH 3/5] Make use of BoolVar instead of creating own type Signed-off-by: Noble Mittal --- go/internal/flag/flag_test.go | 23 ++++++----------------- go/internal/flag/usage_test.go | 17 ++--------------- 2 files changed, 8 insertions(+), 32 deletions(-) diff --git a/go/internal/flag/flag_test.go b/go/internal/flag/flag_test.go index 73e777cac21..1939b6b4892 100644 --- a/go/internal/flag/flag_test.go +++ b/go/internal/flag/flag_test.go @@ -33,7 +33,7 @@ func TestPreventGlogVFlagFromClobberingVersionFlagShorthand(t *testing.T) { goflag.CommandLine = goflag.NewFlagSet(os.Args[0], goflag.ExitOnError) - var v, vtest bool + var v bool goflag.BoolVar(&v, "v", true, "") @@ -44,7 +44,7 @@ func TestPreventGlogVFlagFromClobberingVersionFlagShorthand(t *testing.T) { assert.NotNil(t, f) assert.Equal(t, "", f.Shorthand) - testFlagSet.BoolVar(&vtest, "vtest", true, "") + // The function should not throw any error if -v flag is already defined PreventGlogVFlagFromClobberingVersionFlagShorthand(testFlagSet) } @@ -282,27 +282,16 @@ func TestArgs(t *testing.T) { } func TestIsZeroValue(t *testing.T) { - oldCommandLine := goflag.CommandLine - - defer func() { - goflag.CommandLine = oldCommandLine - }() - var testFlag string - goflag.StringVar(&testFlag, "testflag", "default", "Description of testflag") - goflag.Parse() + testFlagSet := goflag.NewFlagSet("testFlagSet", goflag.ExitOnError) + testFlagSet.StringVar(&testFlag, "testflag", "default", "Description of testflag") - f := goflag.Lookup("testflag") + f := testFlagSet.Lookup("testflag") - // Test the isZeroValue function result := isZeroValue(f, "") - assert.True(t, result) - // Test again with a non-zero value result = isZeroValue(f, "anyValue") - if result { - t.Errorf("Expected false, got true. The value 'newvalue' should not be considered as zero value.") - } + assert.False(t, result) } diff --git a/go/internal/flag/usage_test.go b/go/internal/flag/usage_test.go index d88846f8840..461cd2580ea 100644 --- a/go/internal/flag/usage_test.go +++ b/go/internal/flag/usage_test.go @@ -92,23 +92,10 @@ func TestSetUsageWithNilFlagFilterAndPreface(t *testing.T) { assert.Contains(t, output, "test epilogue") } -type testBool struct{} - -func (t testBool) IsBoolFlag() bool { - return true -} - -func (t testBool) String() string { - return "true" -} - -func (t testBool) Set(s string) error { - return nil -} - func TestSetUsageWithBoolFlag(t *testing.T) { fs := goflag.NewFlagSet("test2", goflag.ExitOnError) - fs.Var(testBool{}, "t", "`t` flag") + var tBool bool + fs.BoolVar(&tBool, "t", true, "`t` flag") opts := UsageOptions{ Preface: func(w io.Writer) { From 60ba1ad0d4cc0d91f8db30cdb41e52e0a2307fc4 Mon Sep 17 00:00:00 2001 From: Noble Mittal Date: Mon, 19 Feb 2024 14:19:14 +0530 Subject: [PATCH 4/5] Add error message for assert.True and assert.False Signed-off-by: Noble Mittal --- go/internal/flag/flag_test.go | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/go/internal/flag/flag_test.go b/go/internal/flag/flag_test.go index 1939b6b4892..b2c31632868 100644 --- a/go/internal/flag/flag_test.go +++ b/go/internal/flag/flag_test.go @@ -44,8 +44,8 @@ func TestPreventGlogVFlagFromClobberingVersionFlagShorthand(t *testing.T) { assert.NotNil(t, f) assert.Equal(t, "", f.Shorthand) - // The function should not throw any error if -v flag is already defined - PreventGlogVFlagFromClobberingVersionFlagShorthand(testFlagSet) + // The function should not panic if -v flag is already defined + assert.NotPanics(t, func() { PreventGlogVFlagFromClobberingVersionFlagShorthand(testFlagSet) }) } func TestParse(t *testing.T) { @@ -96,21 +96,21 @@ func TestIsFlagProvided(t *testing.T) { pflag.CommandLine = pflag.NewFlagSet("testFlagSet", pflag.ExitOnError) isProvided := IsFlagProvided("testFlag") - assert.False(t, isProvided) + assert.False(t, isProvided, "expected IsFlagProvided to return false as provided flag doesn't exist, got true") var testFlag bool pflag.BoolVar(&testFlag, "testFlag", false, "") // Should return false as testFlag is not set isProvided = IsFlagProvided("testFlag") - assert.False(t, isProvided) + assert.False(t, isProvided, "expected IsFlagProvided to return false as no flag was provided, got true") pflag.Parse() _ = pflag.Set("testFlag", "true") // Should return true as testFlag is set isProvided = IsFlagProvided("testFlag") - assert.True(t, isProvided) + assert.True(t, isProvided, "expected IsFlagProvided to return true after providing the flag, got false") } func TestFilterTestFlags(t *testing.T) { @@ -188,11 +188,11 @@ func TestParsed(t *testing.T) { goflag.CommandLine = goflag.NewFlagSet("testGoflagSet", goflag.ExitOnError) b := Parsed() - assert.False(t, b) + assert.False(t, b, "expected Parsed to return false as command-line flags weren't parsed, got true") pflag.Parse() b = Parsed() - assert.True(t, b) + assert.True(t, b, "expected Parsed to return true as command-line flags were parsed, got false") } func TestLookup(t *testing.T) { @@ -290,8 +290,8 @@ func TestIsZeroValue(t *testing.T) { f := testFlagSet.Lookup("testflag") result := isZeroValue(f, "") - assert.True(t, result) + assert.True(t, result, "expected isZeroValue to return true as empty string represents zero value for string flag, got false") result = isZeroValue(f, "anyValue") - assert.False(t, result) + assert.False(t, result, "expected isZeroValue to return false as non-empty string doesn't represent zero value for string flag, got true") } From e37ec38ec28cbc7e5cf07d01bcc1c7487258639f Mon Sep 17 00:00:00 2001 From: Noble Mittal Date: Thu, 29 Feb 2024 23:08:28 +0530 Subject: [PATCH 5/5] Simplify error messages for failing tests Signed-off-by: Noble Mittal --- go/internal/flag/flag_test.go | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/go/internal/flag/flag_test.go b/go/internal/flag/flag_test.go index b2c31632868..1f1ff5dc5ec 100644 --- a/go/internal/flag/flag_test.go +++ b/go/internal/flag/flag_test.go @@ -95,22 +95,23 @@ func TestIsFlagProvided(t *testing.T) { pflag.CommandLine = pflag.NewFlagSet("testFlagSet", pflag.ExitOnError) - isProvided := IsFlagProvided("testFlag") - assert.False(t, isProvided, "expected IsFlagProvided to return false as provided flag doesn't exist, got true") + flagName := "testFlag" + isProvided := IsFlagProvided(flagName) + assert.False(t, isProvided, "flag %q should not exist", flagName) var testFlag bool - pflag.BoolVar(&testFlag, "testFlag", false, "") + pflag.BoolVar(&testFlag, flagName, false, "") // Should return false as testFlag is not set - isProvided = IsFlagProvided("testFlag") - assert.False(t, isProvided, "expected IsFlagProvided to return false as no flag was provided, got true") + isProvided = IsFlagProvided(flagName) + assert.False(t, isProvided, "flag %q should not be provided", flagName) pflag.Parse() - _ = pflag.Set("testFlag", "true") + _ = pflag.Set(flagName, "true") // Should return true as testFlag is set - isProvided = IsFlagProvided("testFlag") - assert.True(t, isProvided, "expected IsFlagProvided to return true after providing the flag, got false") + isProvided = IsFlagProvided(flagName) + assert.True(t, isProvided, "flag %q should be provided", flagName) } func TestFilterTestFlags(t *testing.T) { @@ -188,11 +189,11 @@ func TestParsed(t *testing.T) { goflag.CommandLine = goflag.NewFlagSet("testGoflagSet", goflag.ExitOnError) b := Parsed() - assert.False(t, b, "expected Parsed to return false as command-line flags weren't parsed, got true") + assert.False(t, b, "command-line flags should not be parsed") pflag.Parse() b = Parsed() - assert.True(t, b, "expected Parsed to return true as command-line flags were parsed, got false") + assert.True(t, b, "command-line flags should be parsed") } func TestLookup(t *testing.T) { @@ -290,8 +291,8 @@ func TestIsZeroValue(t *testing.T) { f := testFlagSet.Lookup("testflag") result := isZeroValue(f, "") - assert.True(t, result, "expected isZeroValue to return true as empty string represents zero value for string flag, got false") + assert.True(t, result, "empty string should represent zero value for string flag") result = isZeroValue(f, "anyValue") - assert.False(t, result, "expected isZeroValue to return false as non-empty string doesn't represent zero value for string flag, got true") + assert.False(t, result, "non-empty string should not represent zero value for string flag") }