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

Jest should mark test as 'skipped' if empty function is provided as second argument #2584

Closed
ashtonsix opened this issue Jan 12, 2017 · 15 comments

Comments

@ashtonsix
Copy link
Contributor

ashtonsix commented Jan 12, 2017

This is a feature

Background, current behavior & proposed behavior

When planning tests I would often write code like this:

describe('counter', () => {
  it('updates the value')
  it('re-renders the counter when the value is updated')
})

Until a recent release these examples would crash, I started supplying an empty function as a work-around like so (& expect others do also for various reasons):

it('updates the value', () => {})

Using the latest version of Jest both examples will work, the difference is the first example will create 2 skipped tests & the second example will create a passed test. I believe the tests should be skipped in both cases. Example logic for this might look like:

const shouldSkip = f => {
  if (f === undefined) return true
  f = f.toString()
  if (
    f.test(/\(\s*\)\s*=>\s*{\s*}/) ||
    f.test(/function\s*\(\s*\)\s*{\s*}/)
  ) return true
  return false
}

Using

Windows 7, Jest 17

@thymikee
Copy link
Collaborator

Let's discuss this topic in this issue: #2209 since it refers to the same problem, but with different solutions.

@ashtonsix
Copy link
Contributor Author

@thymikee That's a very different issue. Handling empty functions is unrelated to handling functions with no assertions. Can this be re-opened?

I write lots of tests like this which have no assertions but I certainly don't want to fail or skip where I'm just making sure the code doesn't throw:

it('renders without crashing', () => {
  mount(<Counter />)
})

@thymikee
Copy link
Collaborator

You shouldn't rely on this behaviour, use toThrow/toThrowError/toThrowErrorMatchingSnapshot instead.

@thymikee
Copy link
Collaborator

Reopening this so it can be discussed further.

@ashtonsix
Copy link
Contributor Author

ashtonsix commented Jan 14, 2017

@thymikee I started writing some tests w/o assertions because create-react-app does & @gaearon recommended it on Twitter, you might want to create a relevant issue inside create-react-app to discuss.

Wasn't aware toThrowErrorMatchingSnapshot existed - thanks for letting me know! That's very useful

Happy to create a PR for this issue FYI

@thymikee
Copy link
Collaborator

@cpojer what do you think?

@gaearon
Copy link
Contributor

gaearon commented Jan 14, 2017

You shouldn't rely on this behaviour, use toThrow/toThrowError/toThrowErrorMatchingSnapshot instead.

I wasn’t aware of that, and Create React App has shipped for many versions with test cases that rely on crash failing the test. Is this no longer how it works? Why? Do we need to tell thousands of users to change their test cases? Since which version of Jest has this behavior changed?

@gaearon
Copy link
Contributor

gaearon commented Jan 14, 2017

Oh, I missed the discussion in #2209, sorry. I got the impression that this wasn't implemented.
So do we need to change anything in CRA? I'm a bit confused by this thread.

@thymikee
Copy link
Collaborator

@gaearon cool, nothing has changed 😄. What I mean is that the behaviour of empty tests or tests without expects may change in the future.

As for toThrow or not.toThrow I just personally find it more verbose, but based on this comment #2209 (comment) you can rely on the behaviour currently used in CRA, as expects throw anyway.

#2209 wasn't implemented and is still discussed.

@thymikee
Copy link
Collaborator

Now as I look at this, I like simpler approach of CRA better than not.toThrow.

@gaearon
Copy link
Contributor

gaearon commented Jan 14, 2017

Thanks for clarifying!

@cpojer
Copy link
Member

cpojer commented Jan 15, 2017

@ashtonsix I understand your confusion around the behavior change we made to make tests without a provided function skipped but I don't think we should make it so empty functions are skipped based on the source. It doesn't seem like a reliable method especially when we have an officially supported way for it. I'm sorry you got confused by the feature we added:

test('skipped test');

Also, we will always support tests that simply throw. They will turn into a failing test.

@cpojer cpojer closed this as completed Jan 15, 2017
@Devid-
Copy link

Devid- commented Sep 10, 2017

With the 21 update tests as this will be skipped in jest: it('showTitle');
Is it possible to configure Jest not to skip this test ? Just like in the version 19 ?
The behaviour was interesting for me because I am reading a txt file where each row is a test. And the above test was something like a group title, it was easier to distinguish because it was a yellow circle and all other tests where green or red.
So I had something like this:

○ <<<< General Tests >>>>
✓ general1
✓ general2
✓ general3
✓ general4
○ <<<< Special Tests >>>>
✓ special1
✓ special2
✓ special3
✓ special4

@RobinDaugherty
Copy link

Somehow this discussion got derailed. The request is to have a way to add pending specs without an implementation. That way we can add specs first, then implementations. Ideally, these pending specs would actually appear as "pending". But "skipped" would be sufficient. This supports actual TDD process.

Today, the following:

      it('does not show the Cancel button');

results in the error "Missing second argument. It must be a callback function."

Like I said, this supports TDD, and is implemented in other testing frameworks. For example, rspec.

Passing an "empty function" as the second parameter might be a better developer experience, but is not necessary to implement "pending implementation" examples.

@SimenB
Copy link
Member

SimenB commented Dec 6, 2018

Jest 24 will come with test.todo('my pending test'). From #6996

Install jest@beta to test now.

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

No branches or pull requests

7 participants