franela / goblin

Minimal and Beautiful Go testing framework
MIT License
887 stars 79 forks source link

Request for describe comments to be matched with -goblin.run regexp #105

Open alexey-komarov opened 3 years ago

alexey-komarov commented 3 years ago

I'd like to run particular group of tests described via Describe(), just for developing purposes when I want to debug entire group, so i would really appreciate a separate flag, e.g -goblin.runFullName with the following behavior:

diff --git a/goblin.go b/goblin.go
index a4617db..078965c 100644
--- a/goblin.go
+++ b/goblin.go
@@ -284,8 +284,30 @@ func (g *G) SetReporter(r Reporter) {
        g.reporter = r
 }

+func (g *G) GetFullName(name string) string {
+       parent := g.parent
+       names := []string{name}
+
+       for parent != nil {
+               names = append(names, parent.name)
+               parent = parent.parent
+       }
+
+       result := ""
+       for _, n := range names {
+               if len(result) == 0 {
+                       result = n
+                       continue
+               }
+
+               result = n + "." + result
+       }
+
+       return result
+}
+
 func (g *G) It(name string, h ...interface{}) {
-       if matchesRegex(name) {
+       if matchesRegex(g.GetFullName(name)) {
                it := &It{name: name, parent: g.parent, reporter: g.reporter}
                if g.parent == nil {
                        panic(fmt.Sprintf("It(\"%s\") block should be written inside Describe() block.", name))
marcosnils commented 3 years ago

I like this idea. Do we need a separate flag though? Why not just using the current run flag and just change the current matchesRegex line to use the GetFullName function that you're proposing?

I don't think we're breaking backwards compatibility here, but just allowing to target Describe blocks as it's name will be part of the regex target now.

alexey-komarov commented 3 years ago

@marcosnils Using one flag would be better for sure, but i always think about those who already using this feature in their pipelines, don't want to break anything :)

marcosnils commented 3 years ago

I don't think this is a very big deal since the run flag is mostly used for development purposes and not generally CI pipelines. In any case, we can always release a new major.

Would you like to open a PR so we can get this merged?

One thing that I believe it's missing here is to keep recursing all the way up until parent == nil. Since you can have multiple nested describe blocks: i.e:

Describe("Some", func() {
  Describe("Little", func() {
     Describe("Rabbit", func() {
         It("jumps", func(){})
     })
  })
}) 

So the ultimate run regex should target "Some.Little.Rabbit.jumps" and if I read the code correctly it's only targetting "Rabbit.jumps" given the current patch.

Additionally, does it make sense to concatenate the blocks with .? Why not : or any other char? I don't have any specific preferences here, just wondering if it's the best we can do about it.

shakefu commented 3 years ago

I'd like to see this but concatenated with spaces to match Mocha's fullTitle() behavior