Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/skip it #82

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Feature/skip it #82

wants to merge 2 commits into from

Conversation

sockol
Copy link

@sockol sockol commented Apr 7, 2020

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.

semur nabiev added 2 commits April 6, 2020 21:00
Currently Goblin supports Xit() to skip It() tests.
This adds a different syntax to achieve the same functionality.
@marcosnils
Copy link
Member

marcosnils commented Apr 8, 2020

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
Copy link
Author

sockol commented Apr 8, 2020

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
Copy link
Member

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
Copy link
Author

sockol commented Apr 8, 2020

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
Copy link
Member

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
Copy link
Author

sockol commented Apr 8, 2020

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
Copy link
Author

sockol commented Apr 15, 2020

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

@xetorthio
Copy link
Contributor

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
Copy link
Member

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 🚢

@sockol
Copy link
Author

sockol commented Apr 16, 2020

Awesome, thanks @xetorthio

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants