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

Excluding second argument should not throw, but instead mark the test as skipped #6430

Closed
javorosas opened this issue Jun 10, 2018 · 9 comments · Fixed by #6996
Closed

Excluding second argument should not throw, but instead mark the test as skipped #6430

javorosas opened this issue Jun 10, 2018 · 9 comments · Fixed by #6996

Comments

@javorosas
Copy link

💥 Regression Report

It is a common use case for many teams doing TDD to write the specs first (what the software should do)

it('should sum two numbers');

and implement the behavior later

it('should sum two numbers', () => {
  expect(sum(5, 4)).toBe(9);
});

sometimes these two tasks are even made by two different people.

#5558 introduced throwing an error if the second argument is missing, apparently lacking a proper discussion on the consequences of doing that change.

Last working version

Worked up to version: 22.4.2

Stopped working in version: 23.0.0

To Reproduce

Steps to reproduce the behavior:

Run jest for this test case:

it('should sum two numbers');

Expected behavior

0 tests passed
0 tests failed
1 test skipped

@mattphillips
Copy link
Contributor

Hey @javorosas this issue appears to be the same as #6256

The recommended solution is to add a noop callback:

it.skip('should sum two numbers', () => {});

@rickhanlonii
Copy link
Member

Not a regression as it's listed as a breaking change in the changelog, but I'm cool to revert this and add a lint rule to eslint-plugin-jest instead

We should probably still fail tests that provide a function and no description:

test(() => {});

Since we need the description to give to reporters and track some internal state

@tristan-shelton
Copy link

This one is driving me nuts. Please do at least add a configuration option for this!

@rickhanlonii
Copy link
Member

@tristan-shelton @javorosas we're adding test.todo for this, but if you want to use the previous behavior, here's the config you can use:

// add to setupTestFrameworkScriptFile
// see https://jestjs.io/docs/en/configuration#setuptestframeworkscriptfile-string

const originalTest = test;
test = (description, fn, ...args) => {
  if (!fn) return originalTest(description, () => {});

  return originalTest(description, fn, ...args);
};

test.only = originalTest.only;
test.skip = originalTest.skip;

cc @arcdev1

@tristan-shelton
Copy link

tristan-shelton commented Sep 18, 2018

Thanks for the response @rickhanlonii . While that code will technically work (barring more unexpected api changes) users shouldn't have to resort to such hacks for the most basic expected behaviors of a test runner. The original pull request should never have been accepted, especially not as a default. This is frankly unacceptable.

I've since moved over to Mocha which provides this behavior by default, runs much faster and has far fewer bugs. I would recommend that anyone having this issue do the same until such time as Jest becomes usable again.

@elyobo
Copy link

elyobo commented Sep 18, 2018

The change annoyed me at first, but just providing a noop function is fine once I got in to the habit of it; I'm going to have to have that boilerplate when I write the test anyway, so it's not really even any extra work, it's just doing it at the time I write the stub instead of the test.

As for shifting to mocha for speed... well if that works for you, then good, but I've gone the from mocha to jest for the largely the same reason :D

@tristan-shelton
Copy link

@elyobo Last I checked a no-op function counts as a passing test, not a pending one. If you're writing a spec across multiple test files it's easy to lose track of unfinished tests that way.

@elyobo
Copy link

elyobo commented Sep 19, 2018

I should have been more clear; what you describe is the case for test but not for test.skip, which is what I'm using.

describe(' foo', () => {                                                       
  test.skip('does bar', () => {})                                             
})                                                                              

I stub things out like that so that they're correctly reported as skipped, the function that I'll need to write is there already, and the .skip (which did take a few extra keypresses) makes it easy to find the unimplemented tests by searching for .skip.

A small behavioural change for me, but no real pain. The old behaviour was slightly less work, and the change was to address a problem that I've never encountered myself, but now I'm used to it it's fine and if it helps some others then good.

@github-actions
Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
5 participants