From d6bba86e67d4973f786752a070f69b7732601caa Mon Sep 17 00:00:00 2001 From: Onsi Fakhouri Date: Fri, 16 Jun 2023 11:02:27 -0600 Subject: [PATCH] Programmatic focus is no longer overwrriten by CLI filters --- docs/index.md | 4 +- .../filter_fixture/sprocket_c_test.go | 2 +- .../_fixtures/flags_fixture/flags_test.go | 82 ++++++++----------- integration/filter_test.go | 11 +-- integration/flags_test.go | 22 +++-- internal/focus.go | 71 ++++++++-------- internal/focus_test.go | 35 +++++++- internal/internal_integration/focus_test.go | 45 +++++++++- internal/internal_integration/labels_test.go | 2 +- 9 files changed, 160 insertions(+), 114 deletions(-) diff --git a/docs/index.md b/docs/index.md index 435779528..79da2e9e7 100644 --- a/docs/index.md +++ b/docs/index.md @@ -2597,8 +2597,8 @@ These mechanisms can all be used in concert. They combine with the following ru - `Pending` specs are always pending and can never be coerced to run by another filtering mechanism. - Specs that invoke `Skip()` will always be skipped regardless of other filtering mechanisms. -- The CLI based filters (`--label-filter`, `--focus-file/--skip-file`, `--focus/--skip`) **always** override any programmatic focus. -- When multiple CLI filters are provided they are all ANDed together. The spec must satisfy the label filter query **and** any location-based filters **and** any description based filters. +- Programmatic filters always apply and result in a non-zero exit code. Any additional CLI filters only apply to the subset of specs selected by the programmatic filters. +- When multiple CLI filters (`--label-filter`, `--focus-file/--skip-file`, `--focus/--skip`) are provided they are all ANDed together. The spec must satisfy the label filter query **and** any location-based filters **and** any description based filters. ### Repeating Spec Runs and Managing Flaky Specs diff --git a/integration/_fixtures/filter_fixture/sprocket_c_test.go b/integration/_fixtures/filter_fixture/sprocket_c_test.go index c9bd74369..0c880057b 100644 --- a/integration/_fixtures/filter_fixture/sprocket_c_test.go +++ b/integration/_fixtures/filter_fixture/sprocket_c_test.go @@ -4,7 +4,7 @@ import ( . "github.com/onsi/ginkgo/v2" ) -var _ = FDescribe("SprocketC", func() { +var _ = Describe("SprocketC", func() { It("cat", func() { }) diff --git a/integration/_fixtures/flags_fixture/flags_test.go b/integration/_fixtures/flags_fixture/flags_test.go index 3c2770453..e67a3dbcd 100644 --- a/integration/_fixtures/flags_fixture/flags_test.go +++ b/integration/_fixtures/flags_fixture/flags_test.go @@ -18,66 +18,56 @@ func init() { } var _ = Describe("Testing various flags", func() { - FDescribe("the focused set", func() { - It("should honor -cover", func() { - Ω(Tested()).Should(Equal("tested")) - }) - - It("should allow gcflags", func() { - fmt.Printf("NaN returns %T\n", remapped.NaN()) - }) + It("should honor -cover", func() { + Ω(Tested()).Should(Equal("tested")) + }) - PIt("should honor -failOnPending and -noisyPendings") + It("should allow gcflags", func() { + fmt.Printf("NaN returns %T\n", remapped.NaN()) + }) - Describe("smores", func() { - It("should honor -skip: marshmallow", func() { - println("marshmallow") - }) + PIt("should honor -failOnPending and -noisyPendings") - It("should honor -focus: chocolate", func() { - println("chocolate") - }) + Describe("smores", func() { + It("should honor -skip: marshmallow", func() { + println("marshmallow") }) - It("should detect races", func() { - var a string - c := make(chan interface{}, 0) - go func() { - a = "now you don't" - close(c) - }() - a = "now you see me" - println(a) - Eventually(c).Should(BeClosed()) + It("should honor -focus: chocolate", func() { + println("chocolate") }) + }) - It("should randomize A", func() { - println("RANDOM_A") - }) + It("should detect races", func() { + var a string + c := make(chan interface{}, 0) + go func() { + a = "now you don't" + close(c) + }() + a = "now you see me" + println(a) + Eventually(c).Should(BeClosed()) + }) - It("should randomize B", func() { - println("RANDOM_B") - }) + It("should randomize A", func() { + println("RANDOM_A") + }) - It("should randomize C", func() { - println("RANDOM_C") - }) + It("should randomize B", func() { + println("RANDOM_B") + }) - It("should pass in additional arguments after '--' directly to the test process", func() { - fmt.Printf("CUSTOM_FLAG: %s", customFlag) - }) + It("should randomize C", func() { + println("RANDOM_C") }) - Describe("more smores", func() { - It("should not run these unless -focus is set", func() { - println("smores") - }) + It("should pass in additional arguments after '--' directly to the test process", func() { + fmt.Printf("CUSTOM_FLAG: %s", customFlag) }) - Describe("a failing test", func() { - It("should fail", func() { - Ω(true).Should(Equal(false)) - }) + It("should fail", func() { + Ω(true).Should(Equal(false)) }) Describe("a flaky test", func() { diff --git a/integration/filter_test.go b/integration/filter_test.go index 649a1b3d1..a4f8802a2 100644 --- a/integration/filter_test.go +++ b/integration/filter_test.go @@ -1,15 +1,12 @@ package integration_test import ( - "strings" - . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "github.com/onsi/gomega/gbytes" "github.com/onsi/gomega/gexec" . "github.com/onsi/ginkgo/v2/internal/test_helpers" - "github.com/onsi/ginkgo/v2/types" ) var _ = Describe("Filter", func() { @@ -76,14 +73,10 @@ var _ = Describe("Filter", func() { "--focus=", "--skip=", "--json-report=report.json", ) - Eventually(session).Should(gexec.Exit(types.GINKGO_FOCUS_EXIT_CODE)) + Eventually(session).Should(gexec.Exit(0)) specs := Reports(fm.LoadJSONReports("filter", "report.json")[0].SpecReports) for _, spec := range specs { - if strings.HasPrefix(spec.FullText(), "SprocketC") { - Ω(spec).Should(HavePassed()) - } else { - Ω(spec).Should(Or(HaveBeenSkipped(), BePending())) - } + Ω(spec).Should(SatisfyAny(HavePassed(), BePending())) } }) diff --git a/integration/flags_test.go b/integration/flags_test.go index 1972136d4..b4d63192f 100644 --- a/integration/flags_test.go +++ b/integration/flags_test.go @@ -4,7 +4,6 @@ import ( "strings" . "github.com/onsi/ginkgo/v2" - "github.com/onsi/ginkgo/v2/types" . "github.com/onsi/gomega" "github.com/onsi/gomega/gexec" ) @@ -20,21 +19,20 @@ var _ = Describe("Flags Specs", func() { It("normally passes, prints out noisy pendings, does not randomize tests, and honors the programmatic focus", func() { session := startGinkgo(fm.PathTo("flags"), "--no-color") - Eventually(session).Should(gexec.Exit(types.GINKGO_FOCUS_EXIT_CODE)) + Eventually(session).Should(gexec.Exit(1)) output := string(session.Out.Contents()) Ω(output).Should(ContainSubstring("9 Passed")) - Ω(output).Should(ContainSubstring("0 Failed")) + Ω(output).Should(ContainSubstring("2 Failed")) Ω(output).Should(ContainSubstring("1 Pending")) - Ω(output).Should(ContainSubstring("3 Skipped")) + Ω(output).Should(ContainSubstring("0 Skipped")) Ω(output).Should(ContainSubstring("[PENDING]")) Ω(output).Should(ContainSubstring("marshmallow")) Ω(output).Should(ContainSubstring("chocolate")) Ω(output).Should(ContainSubstring("CUSTOM_FLAG: default")) - Ω(output).Should(ContainSubstring("Detected Programmatic Focus - setting exit status to %d", types.GINKGO_FOCUS_EXIT_CODE)) - Ω(output).ShouldNot(ContainSubstring("smores")) Ω(output).ShouldNot(ContainSubstring("SLOW TEST")) Ω(output).ShouldNot(ContainSubstring("should honor -slow-spec-threshold")) + Ω(output).ShouldNot(ContainSubstring("Full Stack Trace")) orders := getRandomOrders(output) Ω(orders[0]).Should(BeNumerically("<", orders[1])) @@ -53,15 +51,15 @@ var _ = Describe("Flags Specs", func() { Skip("race detection is not supported") } session := startGinkgo(fm.PathTo("flags"), "--no-color", "--race") - Eventually(session).Should(gexec.Exit(types.GINKGO_FOCUS_EXIT_CODE)) + Eventually(session).Should(gexec.Exit(1)) output := string(session.Out.Contents()) Ω(output).Should(ContainSubstring("WARNING: DATA RACE")) }) It("should randomize tests when told to", func() { - session := startGinkgo(fm.PathTo("flags"), "--no-color", "--randomize-all", "--seed=40") - Eventually(session).Should(gexec.Exit(types.GINKGO_FOCUS_EXIT_CODE)) + session := startGinkgo(fm.PathTo("flags"), "--no-color", "--randomize-all", "--seed=120") + Eventually(session).Should(gexec.Exit(1)) output := string(session.Out.Contents()) orders := getRandomOrders(output) @@ -70,14 +68,14 @@ var _ = Describe("Flags Specs", func() { It("should pass additional arguments in", func() { session := startGinkgo(fm.PathTo("flags"), "--", "--customFlag=madagascar") - Eventually(session).Should(gexec.Exit(types.GINKGO_FOCUS_EXIT_CODE)) + Eventually(session).Should(gexec.Exit(1)) output := string(session.Out.Contents()) Ω(output).Should(ContainSubstring("CUSTOM_FLAG: madagascar")) }) It("should print out full stack traces for failures when told to", func() { - session := startGinkgo(fm.PathTo("flags"), "--focus=a failing test", "--trace") + session := startGinkgo(fm.PathTo("flags"), "--trace") Eventually(session).Should(gexec.Exit(1)) output := string(session.Out.Contents()) @@ -148,7 +146,7 @@ var _ = Describe("Flags Specs", func() { It("should emit node start/end events when running with --show-node-events", func() { session := startGinkgo(fm.PathTo("flags"), "--no-color", "-v", "--show-node-events") - Eventually(session).Should(gexec.Exit(types.GINKGO_FOCUS_EXIT_CODE)) + Eventually(session).Should(gexec.Exit(1)) output := string(session.Out.Contents()) Eventually(output).Should(ContainSubstring("> Enter [It] should honor -cover")) diff --git a/internal/focus.go b/internal/focus.go index 966ea0c1a..e3da7d14d 100644 --- a/internal/focus.go +++ b/internal/focus.go @@ -8,22 +8,22 @@ import ( ) /* - If a container marked as focus has a descendant that is also marked as focus, Ginkgo's policy is to - unmark the container's focus. This gives developers a more intuitive experience when debugging specs. - It is common to focus a container to just run a subset of specs, then identify the specific specs within the container to focus - - this policy allows the developer to simply focus those specific specs and not need to go back and turn the focus off of the container: - - As a common example, consider: - - FDescribe("something to debug", function() { - It("works", function() {...}) - It("works", function() {...}) - FIt("doesn't work", function() {...}) - It("works", function() {...}) - }) - - here the developer's intent is to focus in on the `"doesn't work"` spec and not to run the adjacent specs in the focused `"something to debug"` container. - The nested policy applied by this function enables this behavior. +If a container marked as focus has a descendant that is also marked as focus, Ginkgo's policy is to +unmark the container's focus. This gives developers a more intuitive experience when debugging specs. +It is common to focus a container to just run a subset of specs, then identify the specific specs within the container to focus - +this policy allows the developer to simply focus those specific specs and not need to go back and turn the focus off of the container: + +As a common example, consider: + + FDescribe("something to debug", function() { + It("works", function() {...}) + It("works", function() {...}) + FIt("doesn't work", function() {...}) + It("works", function() {...}) + }) + +here the developer's intent is to focus in on the `"doesn't work"` spec and not to run the adjacent specs in the focused `"something to debug"` container. +The nested policy applied by this function enables this behavior. */ func ApplyNestedFocusPolicyToTree(tree *TreeNode) { var walkTree func(tree *TreeNode) bool @@ -44,46 +44,43 @@ func ApplyNestedFocusPolicyToTree(tree *TreeNode) { } /* - Ginkgo supports focussing specs using `FIt`, `FDescribe`, etc. - this is called "programmatic focus" - It also supports focussing specs using regular expressions on the command line (`-focus=`, `-skip=`) that match against spec text - and file filters (`-focus-files=`, `-skip-files=`) that match against code locations for nodes in specs. +Ginkgo supports focussing specs using `FIt`, `FDescribe`, etc. - this is called "programmatic focus" +It also supports focussing specs using regular expressions on the command line (`-focus=`, `-skip=`) that match against spec text and file filters (`-focus-files=`, `-skip-files=`) that match against code locations for nodes in specs. - If any of the CLI flags are provided they take precedence. The file filters run first followed by the regex filters. +When both programmatic and file filters are provided their results are ANDed together. If multiple kinds of filters are provided, the file filters run first followed by the regex filters. - This function sets the `Skip` property on specs by applying Ginkgo's focus policy: - - If there are no CLI arguments and no programmatic focus, do nothing. - - If there are no CLI arguments but a spec somewhere has programmatic focus, skip any specs that have no programmatic focus. - - If there are CLI arguments parse them and skip any specs that either don't match the focus filters or do match the skip filters. +This function sets the `Skip` property on specs by applying Ginkgo's focus policy: +- If there are no CLI arguments and no programmatic focus, do nothing. +- If a spec somewhere has programmatic focus skip any specs that have no programmatic focus. +- If there are CLI arguments parse them and skip any specs that either don't match the focus filters or do match the skip filters. - *Note:* specs with pending nodes are Skipped when created by NewSpec. +*Note:* specs with pending nodes are Skipped when created by NewSpec. */ func ApplyFocusToSpecs(specs Specs, description string, suiteLabels Labels, suiteConfig types.SuiteConfig) (Specs, bool) { focusString := strings.Join(suiteConfig.FocusStrings, "|") skipString := strings.Join(suiteConfig.SkipStrings, "|") - hasFocusCLIFlags := focusString != "" || skipString != "" || len(suiteConfig.SkipFiles) > 0 || len(suiteConfig.FocusFiles) > 0 || suiteConfig.LabelFilter != "" - type SkipCheck func(spec Spec) bool // by default, skip any specs marked pending skipChecks := []SkipCheck{func(spec Spec) bool { return spec.Nodes.HasNodeMarkedPending() }} hasProgrammaticFocus := false - if !hasFocusCLIFlags { - // check for programmatic focus - for _, spec := range specs { - if spec.Nodes.HasNodeMarkedFocus() && !spec.Nodes.HasNodeMarkedPending() { - skipChecks = append(skipChecks, func(spec Spec) bool { return !spec.Nodes.HasNodeMarkedFocus() }) - hasProgrammaticFocus = true - break - } + for _, spec := range specs { + if spec.Nodes.HasNodeMarkedFocus() && !spec.Nodes.HasNodeMarkedPending() { + hasProgrammaticFocus = true + break } } + if hasProgrammaticFocus { + skipChecks = append(skipChecks, func(spec Spec) bool { return !spec.Nodes.HasNodeMarkedFocus() }) + } + if suiteConfig.LabelFilter != "" { labelFilter, _ := types.ParseLabelFilter(suiteConfig.LabelFilter) - skipChecks = append(skipChecks, func(spec Spec) bool { - return !labelFilter(UnionOfLabels(suiteLabels, spec.Nodes.UnionOfLabels())) + skipChecks = append(skipChecks, func(spec Spec) bool { + return !labelFilter(UnionOfLabels(suiteLabels, spec.Nodes.UnionOfLabels())) }) } diff --git a/internal/focus_test.go b/internal/focus_test.go index 5ac30bb0e..99f08504c 100644 --- a/internal/focus_test.go +++ b/internal/focus_test.go @@ -155,7 +155,7 @@ var _ = Describe("Focus", func() { S(N("green dragon"), N()), S(N(Pending), N("blue Dragon")), S(N("yellow dragon")), - S(N(Focus, "yellow dragon")), + S(N("yellow dragon")), } }) @@ -218,7 +218,7 @@ var _ = Describe("Focus", func() { S(N(CL("file_b", 3, "file_b", 15))), //include because "file_:15-21" is in FocusFiles S(N(CL("file_b", 17))), //skip because "_b:17" is in SkipFiles S(N(CL("file_b", 20), Pending)), //skip because spec is flagged pending - S(N(CL("c", 3), Focus)), //skip because "c" is not in FocusFiles - override programmatic focus + S(N(CL("c", 3))), //skip because "c" is not in FocusFiles S(N(CL("d", 17))), //include because "d " is in FocusFiles } @@ -240,7 +240,7 @@ var _ = Describe("Focus", func() { S(N(ntCon, Label("cat", "dog")), N(ntIt, "A", Label("fish"))), //skip because fish S(N(ntCon, Label("cat", "dog")), N(ntIt, "B", Label("apple"))), //include because has cat and not fish S(N(ntCon, Label("dog")), N(ntIt, "C", Label("apple"))), //skip because no cat or cow - S(N(ntCon, Label("cow")), N(ntIt, "D", Label("fish"), Focus)), //skip because fish, override focus + S(N(ntCon, Label("cow")), N(ntIt, "D", Label("fish"))), //skip because fish S(N(ntCon, Label("cow")), N(ntIt, "E")), //include because cow and no fish S(N(ntCon, Label("cow")), N(ntIt, "F", Pending)), //skip because pending } @@ -277,7 +277,7 @@ var _ = Describe("Focus", func() { S(N("dog cat", CL("file_b", 3, "file_b", 15), Label("brown"))), //skip because "file_:15-21" is in FocusFiles but "cat" is in SkipStirngs S(N("fish", CL("file_b", 17), Label("brown"))), //skip because "_b:17" is in SkipFiles, even though "fish" is in FocusStrings S(N("biscuit", CL("file_b", 20), Pending, Label("brown"))), //skip because spec is flagged pending - S(N("pony", CL("c", 3), Focus, Label("brown"))), //skip because "c" is not in FocusFiles or FocusStrings - override programmatic focus + S(N("pony", CL("c", 3), Label("brown"))), //skip because "c" is not in FocusFiles or FocusStrings S(N("goat", CL("d", 17), Label("brown"))), //skip because "goat" is in FocusStrings but "d" is not in FocusFiles } @@ -294,5 +294,32 @@ var _ = Describe("Focus", func() { Ω(hasProgrammaticFocus).Should(BeFalse()) }) }) + + Context("when configured with focus/skip files, focus/skip strings, and label filters and there is a programmatic focus", func() { + BeforeEach(func() { + specs = Specs{ + S(N("dog", CL("file_a", 1), Label("brown"))), //skip because "file_:1" is in FocusFiles and "dog" is in FocusStrings and has "brown" label but a different spec has a programmatic focus + S(N("dog", CL("file_a", 17), Label("brown"), Focus)), //include because "file_:15-21" is in FocusFiles and "dog" is in FocusStrings and has "brown" label + S(N("dog", CL("file_a", 1), Label("white"), Focus)), //skip because does not have "brown" label + S(N("dog cat", CL("file_b", 3, "file_b", 15), Label("brown"))), //skip because "file_:15-21" is in FocusFiles but "cat" is in SkipStirngs + S(N("fish", CL("file_b", 17), Label("brown"))), //skip because "_b:17" is in SkipFiles, even though "fish" is in FocusStrings + S(N("biscuit", CL("file_b", 20), Pending, Label("brown"))), //skip because spec is flagged pending + S(N("pony", CL("c", 3), Label("brown"))), //skip because "c" is not in FocusFiles or FocusStrings + S(N("goat", CL("d", 17), Label("brown"))), //skip because "goat" is in FocusStrings but "d" is not in FocusFiles + } + + conf.FocusFiles = []string{"file_:1,15-21"} + conf.SkipFiles = []string{"_b:17"} + conf.FocusStrings = []string{"goat", "dog", "fish", "biscuit"} + conf.SkipStrings = []string{"cat"} + conf.LabelFilter = "brown" + }) + + It("applies all filters", func() { + specs, hasProgrammaticFocus := internal.ApplyFocusToSpecs(specs, description, suiteLabels, conf) + Ω(harvestSkips(specs)).Should(Equal([]bool{true, false, true, true, true, true, true, true})) + Ω(hasProgrammaticFocus).Should(BeTrue()) + }) + }) }) }) diff --git a/internal/internal_integration/focus_test.go b/internal/internal_integration/focus_test.go index 7cfb531c5..0eadb35bb 100644 --- a/internal/internal_integration/focus_test.go +++ b/internal/internal_integration/focus_test.go @@ -122,7 +122,7 @@ var _ = Describe("Focus", func() { BeforeEach(func() { conf.FocusStrings = []string{"blue", "green"} conf.SkipStrings = []string{"red"} - success, _ := RunFixture("cli focus tests", func() { + success, hasProgrammaticFocus := RunFixture("cli focus tests", func() { It("blue.1", rt.T("blue.1")) It("blue.2", rt.T("blue.2")) Describe("blue.container", func() { @@ -137,9 +137,10 @@ var _ = Describe("Focus", func() { Describe("red.2", func() { It("green.2", rt.T("green.2")) }) - FIt("red.3", rt.T("red.3")) + It("red.3", rt.T("red.3")) }) Ω(success).Should(BeTrue()) + Ω(hasProgrammaticFocus).Should(BeFalse()) }) It("should run tests that match", func() { @@ -157,6 +158,46 @@ var _ = Describe("Focus", func() { }) }) + Describe("with a combination of programmatic focus and config.FocusStrings and config.SkipStrings", func() { + BeforeEach(func() { + conf.FocusStrings = []string{"blue", "green"} + conf.SkipStrings = []string{"red"} + success, hasProgrammaticFocus := RunFixture("cli focus tests", func() { + It("blue.1", rt.T("blue.1")) + FIt("blue.2", rt.T("blue.2")) + FDescribe("blue.container", func() { + It("yellow.1", rt.T("yellow.1")) + It("red.1", rt.T("red.1")) + PIt("blue.3", rt.T("blue.3")) + }) + Describe("green.container", func() { + It("yellow.2", rt.T("yellow.2")) + It("green.1", rt.T("green.1")) + }) + FDescribe("red.2", func() { + It("green.2", rt.T("green.2")) + }) + FIt("red.3", rt.T("red.3")) + }) + Ω(success).Should(BeTrue()) + Ω(hasProgrammaticFocus).Should(BeTrue()) + }) + + It("should run tests that match all the filters", func() { + Ω(rt.TrackedRuns()).Should(ConsistOf("blue.2", "yellow.1")) + }) + + It("should report on the tests correctly", func() { + Ω(reporter.Did.WithState(types.SpecStateSkipped).Names()).Should(ConsistOf("blue.1", "red.1", "yellow.2", "green.1", "green.2", "red.3")) + Ω(reporter.Did.WithState(types.SpecStatePending).Names()).Should(ConsistOf("blue.3")) + Ω(reporter.Did.WithState(types.SpecStatePassed).Names()).Should(ConsistOf("blue.2", "yellow.1")) + }) + + It("report on the suite with accurate numbers", func() { + Ω(reporter.End).Should(BeASuiteSummary(true, NPassed(2), NSkipped(6), NPending(1), NSpecs(9), NWillRun(2))) + }) + }) + Describe("when no tests will end up running", func() { BeforeEach(func() { conf.FocusStrings = []string{"red"} diff --git a/internal/internal_integration/labels_test.go b/internal/internal_integration/labels_test.go index f51a5591d..09811c49e 100644 --- a/internal/internal_integration/labels_test.go +++ b/internal/internal_integration/labels_test.go @@ -12,7 +12,7 @@ var _ = Describe("Labels", func() { Describe("when a suite has labelled tests", func() { fixture := func() { Describe("outer container", func() { - It("A", rt.T("A"), Focus, Label("cat")) + It("A", rt.T("A"), Label("cat")) It("B", rt.T("B"), Label("dog")) Describe("container", Label("cow", "cat"), func() { It("C", rt.T("C"))