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 Request: Xit() to Exclude Test #62

Merged
merged 8 commits into from
Aug 21, 2017
Merged

Feature Request: Xit() to Exclude Test #62

merged 8 commits into from
Aug 21, 2017

Conversation

dan9186
Copy link
Contributor

@dan9186 dan9186 commented Aug 21, 2017

I saw issue #56 has been around for a while with the comment of not having time to add it, and noticed #60 came up recently. Thought I would take a stab at adding it.

  • Adds Itable interface for variations of It
  • Adds Xit struct and methods
  • Adds Yellow fancier for excluded outputs
  • Adds excluded output to reports
  • Updates README.md to include new functionality

Copy link
Member

@marcosnils marcosnils left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor comment in tests, otherwise LGTM!.

goblin_test.go Outdated

})

if fakeTest.Failed() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be nice to also assert that count is still 0 in the end.

@dan9186
Copy link
Contributor Author

dan9186 commented Aug 21, 2017

Good idea. Added an extra check for making sure count is not altered.

@marcosnils marcosnils merged commit f9d2e96 into franela:master Aug 21, 2017
@marcosnils
Copy link
Member

Thanks a lot for the contribution @dan9186 . We'll move the images to static hosting soon :)

This was referenced Aug 21, 2017
@jayd3e
Copy link

jayd3e commented Aug 21, 2017

Why was Xit chosen instead of something like Ignore, @dan9186? It strikes me as far less clear.

@dan9186
Copy link
Contributor Author

dan9186 commented Aug 22, 2017

With a conversation already existing where the project owner expressed acceptance of Xit and the project's goal of matching behavior to Mocha, I opted for what seemed to be most inline with those two points.

@jayd3e
Copy link

jayd3e commented Aug 23, 2017

Those reasons make sense! Thanks for the explanation. Already using this feature 👌 . Thanks!

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