franela / goblin

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

Feature/skip it #82

Open sockol opened 4 years ago

sockol commented 4 years ago

Goblin is inspired by Mocha, and Mocha has a set of functions that allows you to skip tests. Goblin supports skipping with Xit().

Xit("skip this", func(){})

Mocha allows you to invoke tests with a skip() function:

it.skip("skip this", function(){})

Since Go does not allow us to have a struct and a function with the same name we cannot have

g.It()// It is a function attached to the G struct
g.It.Skip() // It is a nested struct in G

Alternatively, and as a way to set up skipping Describe blocks in a future PR, we can follow this pattern:

// skip this It test
g.Skip.It("skip this", func(){})
// skip all tests in this block 
g.Skip.Describe("skip these", func(){
  g.It("skip this", func(){})
})

Xit() functionality is working as is for backwards compatibility.

marcosnils commented 4 years ago

IDK about this feature? What's the reason reason to change the api and make it more Mocha similar? I'm just wondering if it really makes sense to introduce a new API at this point.

I believe I'd like more opinions about this. @xetorthio @matipan ?

sockol commented 4 years ago

I was thinking of adding an Only functionality that would act similar to this: https://mochajs.org/#exclusive-tests

The main rationale is to have a consistent interface to skip tests for It and Describe. That would follow the same pattern of g.[MODIFIER].[It|Describe]

g.Skip.It()
g.Skip.Describe()
g.Only.It()
g.Only.Describe()
marcosnils commented 4 years ago

Hmm.. interesting. I like the concept but I'm not sure how useful that is.

Only can be easily replaced by the -goblin.run flag. I'm wondering if the Mocha community find this feature really useful. Would love to search in GH to see if devs are actually using that :D

sockol commented 4 years ago

I definitely used it.only() when I used to work with Node, but I doubt we will find too many cases of it.only() on github being used in production code since it's something you use while testing, and once the test passes, you change it.only() to it()

https://github.com/search?l=JavaScript&p=5&q=%22it.only%22&type=Code

I do not know about the most popular testing frameworks from other languages, but in JS the .only pattern seems popular: https://github.com/search?l=JavaScript&p=5&q=%22it.only%22&type=Code

marcosnils commented 4 years ago

I definitely used it.only() when I used to work with Node, but I doubt we will find too many cases of it.only() on github being used in production code since it's something you use while testing, and once the test passes, you change it.only() to it()

I see.. and what would be the difference to use it.only() vs -goblin.run in this case? The fact that it's "easier" to set different it and describe blocks?

sockol commented 4 years ago

I would say the biggest difference is not having to rely on regex to run any given test, or group of tests, while you are working on a separate piece of functionality.

The only functional difference here is being able to test multiple sets of tests within a describe block without having to change the regex to include every It test we want to run.

With regex go test -goblin.run="TestThis|ThatTestAlso|And also this test here"

With Only

g.Only.It("TestThis", func(){})
g.Only.It("ThatTestAlso", func(){})
g.Only.It("And also this test here", func(){})
sockol commented 4 years ago

I don't think we're getting a review @marcosnils

xetorthio commented 4 years ago

Sorry for the delay. Just had time to check this today.

If I get it right this PR as it is right now doesn't fix anything or add any new feature. It only add a different way of skipping It blocks. I do prefer this approach but I don't think we should merge this PR as it is as it since it is not really adding any new feature or fixing anything.

Now... skipping Describe blocks and adding exclusive tests is something that people could benefit from. And I do believe that how @sockol is proposing to solve it is elegant and closer to how mocha does it.

So I think that we should keep this PR open and work on adding one of those features and merge it then. And eventually, in a future major release, consider deprecating Xit.

marcosnils commented 4 years ago

Now... skipping Describe blocks and adding exclusive tests is something that people could benefit from. And I do believe that how @sockol is proposing to solve it is elegant and closer to how mocha does it.

SGTM! Let's add those features also and :ship:

sockol commented 4 years ago

Awesome, thanks @xetorthio