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

Better error message when a describe block is empty #6372

Merged
merged 10 commits into from
Oct 7, 2018
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@
- `[jest-jasmine]` Show proper error message from async `assert` errors ([#6821](https://github.com/facebook/jest/pull/6821))
- `[jest-circus]` Fail synchronous hook timeouts ([#7074](https://github.com/facebook/jest/pull/7074))
- `[jest-jasmine2]` Fail synchronous test timeouts ([#7074](https://github.com/facebook/jest/pull/7074))
- `[jest-jasmine2]` Better error message when a describe block is empty ([#6372](https://github.com/facebook/jest/pull/6372))
- `[jest-circus]` Better error message when a describe block is empty ([#6372](https://github.com/facebook/jest/pull/6372))

### Chore & Maintenance

Expand Down
15 changes: 2 additions & 13 deletions e2e/Utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -119,14 +119,7 @@ const createEmptyPackage = (
);
};

type ExtractSummaryOptions = {|
stripLocation: boolean,
|};

const extractSummary = (
stdout: string,
{stripLocation = false}: ExtractSummaryOptions = {},
) => {
const extractSummary = (stdout: string) => {
const match = stdout.match(
/Test Suites:.*\nTests.*\nSnapshots.*\nTime.*(\nRan all test suites)*.*\n*$/gm,
);
Expand All @@ -144,15 +137,11 @@ const extractSummary = (
.replace(/\d*\.?\d+m?s/g, '<<REPLACED>>')
.replace(/, estimated <<REPLACED>>/g, '');

let rest = cleanupStackTrace(
const rest = cleanupStackTrace(
// remove all timestamps
stdout.replace(match[0], '').replace(/\s*\(\d*\.?\d+m?s\)$/gm, ''),
);

if (stripLocation) {
rest = rest.replace(/(at .*):\d+:\d+/g, '$1:<<LINE>>:<<COLUMN>>');
}

return {rest, summary};
};

Expand Down
25 changes: 25 additions & 0 deletions e2e/__tests__/__snapshots__/globals.test.js.snap
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,31 @@ Time: <<REPLACED>>
Ran all test suites."
`;

exports[`cannot have describe with no implementation 1`] = `
"FAIL __tests__/only-constructs.test.js
● Test suite failed to run

Missing second argument. It must be a callback function.

1 |
> 2 | describe('describe, no implementation');
| ^
3 |

at __tests__/only-constructs.test.js:2:5

"
`;

exports[`cannot have describe with no implementation 2`] = `
"Test Suites: 1 failed, 1 total
Tests: 0 total
Snapshots: 0 total
Time: <<REPLACED>>
Ran all test suites.
"
`;

exports[`cannot test with no implementation 1`] = `
"FAIL __tests__/only-constructs.test.js
● Test suite failed to run
Expand Down
18 changes: 17 additions & 1 deletion e2e/__tests__/globals.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ const TEST_DIR = path.resolve(DIR, '__tests__');

function cleanStderr(stderr) {
const {rest} = extractSummary(stderr);
return rest.replace(/.*(jest-jasmine2|jest-circus).*\n/g, '');
return rest.replace(/.*(jest-jasmine2).*\n/g, '');
Copy link
Member

@SimenB SimenB Oct 7, 2018

Choose a reason for hiding this comment

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

no need to strip out circus stuff as we always create an error without any circus traces in it. Can't be bothered doing the same fix for jasmine, though

}

beforeEach(() => {
Expand Down Expand Up @@ -112,6 +112,22 @@ test('only', () => {
expect(summary).toMatchSnapshot();
});

test('cannot have describe with no implementation', () => {
const filename = 'only-constructs.test.js';
const content = `
describe('describe, no implementation');
`;

writeFiles(TEST_DIR, {[filename]: content});
const {stderr, status} = runJest(DIR);
expect(status).toBe(1);

const rest = cleanStderr(stderr);
const {summary} = extractSummary(stderr);
expect(rest).toMatchSnapshot();
expect(summary).toMatchSnapshot();
});

test('cannot test with no implementation', () => {
const filename = 'only-constructs.test.js';
const content = `
Expand Down
20 changes: 10 additions & 10 deletions packages/jest-circus/src/__tests__/circus_it_test_error.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,24 +35,24 @@ describe('test/it error throwing', () => {
});
it(`it throws error with missing callback function`, () => {
expect(() => {
// $FlowFixMe: Easy, we're testing runitme errors here
// $FlowFixMe: Easy, we're testing runtime errors here
circusIt('test2');
}).toThrowError(
'Missing second argument. It must be a callback function. Perhaps you want to use `test.todo` for a test placeholder.',
);
});
it(`it throws an error when first argument isn't a string`, () => {
expect(() => {
// $FlowFixMe: Easy, we're testing runitme errors here
// $FlowFixMe: Easy, we're testing runtime errors here
circusIt(() => {});
}).toThrowError(`Invalid first argument, () => {}. It must be a string.`);
}).toThrowError('Invalid first argument, () => {}. It must be a string.');
});
it('it throws an error when callback function is not a function', () => {
expect(() => {
// $FlowFixMe: Easy, we're testing runitme errors here
// $FlowFixMe: Easy, we're testing runtime errors here
circusIt('test4', 'test4b');
}).toThrowError(
`Invalid second argument, test4b. It must be a callback function.`,
'Invalid second argument, test4b. It must be a callback function.',
);
});
it(`test doesn't throw an error with valid arguments`, () => {
Expand All @@ -62,24 +62,24 @@ describe('test/it error throwing', () => {
});
it(`test throws error with missing callback function`, () => {
expect(() => {
// $FlowFixMe: Easy, we're testing runitme errors here
// $FlowFixMe: Easy, we're testing runtime errors here
circusTest('test6');
}).toThrowError(
'Missing second argument. It must be a callback function. Perhaps you want to use `test.todo` for a test placeholder.',
);
});
it(`test throws an error when first argument isn't a string`, () => {
expect(() => {
// $FlowFixMe: Easy, we're testing runitme errors here
// $FlowFixMe: Easy, we're testing runtime errors here
circusTest(() => {});
}).toThrowError(`Invalid first argument, () => {}. It must be a string.`);
}).toThrowError('Invalid first argument, () => {}. It must be a string.');
});
it('test throws an error when callback function is not a function', () => {
expect(() => {
// $FlowFixMe: Easy, we're testing runitme errors here
// $FlowFixMe: Easy, we're testing runtime errors here
circusTest('test8', 'test8b');
}).toThrowError(
`Invalid second argument, test8b. It must be a callback function.`,
'Invalid second argument, test8b. It must be a callback function.',
);
});
});
127 changes: 63 additions & 64 deletions packages/jest-circus/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import type {
BlockMode,
BlockName,
TestName,
TestMode,
} from 'types/Circus';
import {bind as bindEach} from 'jest-each';
import {ErrorWithStack} from 'jest-util';
Expand All @@ -23,15 +24,29 @@ import {dispatch} from './state';
type THook = (fn: HookFn, timeout?: number) => void;

const describe = (blockName: BlockName, blockFn: BlockFn) =>
_dispatchDescribe(blockFn, blockName);
_dispatchDescribe(blockFn, blockName, describe);
describe.only = (blockName: BlockName, blockFn: BlockFn) =>
_dispatchDescribe(blockFn, blockName, 'only');
_dispatchDescribe(blockFn, blockName, describe.only, 'only');
describe.skip = (blockName: BlockName, blockFn: BlockFn) =>
_dispatchDescribe(blockFn, blockName, 'skip');

const _dispatchDescribe = (blockFn, blockName, mode?: BlockMode) => {
_dispatchDescribe(blockFn, blockName, describe.skip, 'skip');

const _dispatchDescribe = (
blockFn,
blockName,
describeFn,
mode?: BlockMode,
) => {
const asyncError = new ErrorWithStack(undefined, describeFn);
if (blockFn === undefined) {
asyncError.message = `Missing second argument. It must be a callback function.`;
throw asyncError;
}
if (typeof blockFn !== 'function') {
asyncError.message = `Invalid second argument, ${blockFn}. It must be a callback function.`;
throw asyncError;
}
dispatch({
asyncError: new Error(),
asyncError,
blockName,
mode,
name: 'start_describe_definition',
Expand All @@ -41,11 +56,15 @@ const _dispatchDescribe = (blockFn, blockName, mode?: BlockMode) => {
};

const _addHook = (fn: HookFn, hookType: HookType, hookFn, timeout: ?number) => {
const asyncError = new ErrorWithStack(undefined, hookFn);

if (typeof fn !== 'function') {
throw new Error('Invalid first argument. It must be a callback function.');
asyncError.message =
'Invalid first argument. It must be a callback function.';

throw asyncError;
}

const asyncError = new ErrorWithStack(undefined, hookFn);
dispatch({asyncError, fn, hookType, name: 'add_hook', timeout});
};

Expand All @@ -59,76 +78,56 @@ const afterEach: THook = (fn, timeout) =>
const afterAll: THook = (fn, timeout) =>
_addHook(fn, 'afterAll', afterAll, timeout);

const test = (testName: TestName, fn: TestFn, timeout?: number) => {
if (typeof testName !== 'string') {
throw new Error(
`Invalid first argument, ${testName}. It must be a string.`,
);
}
if (fn === undefined) {
throw new Error(
'Missing second argument. It must be a callback function. Perhaps you want to use `test.todo` for a test placeholder.',
);
}
if (typeof fn !== 'function') {
throw new Error(
`Invalid second argument, ${fn}. It must be a callback function.`,
);
}

const asyncError = new ErrorWithStack(undefined, test);

return dispatch({
asyncError,
fn,
name: 'add_test',
testName,
timeout,
});
};
const test = (testName: TestName, fn: TestFn, timeout?: number) =>
_addTest(testName, undefined, fn, test, timeout);
const it = test;
test.skip = (testName: TestName, fn?: TestFn, timeout?: number) => {
const asyncError = new ErrorWithStack(undefined, test);

return dispatch({
asyncError,
fn,
mode: 'skip',
name: 'add_test',
testName,
timeout,
});
};
test.only = (testName: TestName, fn: TestFn, timeout?: number) => {
const asyncError = new ErrorWithStack(undefined, test);

return dispatch({
asyncError,
fn,
mode: 'only',
name: 'add_test',
testName,
timeout,
});
};

test.skip = (testName: TestName, fn?: TestFn, timeout?: number) =>
_addTest(testName, 'skip', fn, test.skip, timeout);
test.only = (testName: TestName, fn: TestFn, timeout?: number) =>
_addTest(testName, 'only', fn, test.only, timeout);
test.todo = (testName: TestName, ...rest: Array<mixed>) => {
if (rest.length > 0 || typeof testName !== 'string') {
throw new ErrorWithStack(
'Todo must be called with only a description.',
test.todo,
);
}
return _addTest(testName, 'todo', () => {}, test.todo);
};

const _addTest = (
testName: TestName,
mode: TestMode,
fn?: TestFn,
testFn,
timeout: ?number,
) => {
const asyncError = new ErrorWithStack(undefined, testFn);

if (typeof testName !== 'string') {
asyncError.message = `Invalid first argument, ${testName}. It must be a string.`;

const asyncError = new ErrorWithStack(undefined, test);
throw asyncError;
}
if (fn === undefined) {
asyncError.message =
'Missing second argument. It must be a callback function. Perhaps you want to use `test.todo` for a test placeholder.';

throw asyncError;
}
if (typeof fn !== 'function') {
asyncError.message = `Invalid second argument, ${fn}. It must be a callback function.`;

throw asyncError;
}

return dispatch({
asyncError,
fn: () => {},
mode: 'todo',
fn,
mode,
name: 'add_test',
testName,
timeout: undefined,
timeout,
});
};

Expand Down
4 changes: 2 additions & 2 deletions packages/jest-jasmine2/src/__tests__/it_test_error.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ describe('test/it error throwing', () => {
expect(() => {
it('test3', 'test3b');
}).toThrowError(
`Invalid second argument, test3b. It must be a callback function.`,
'Invalid second argument, test3b. It must be a callback function.',
);
});
test(`test throws error with missing callback function`, () => {
Expand All @@ -44,7 +44,7 @@ describe('test/it error throwing', () => {
expect(() => {
test('test6', 'test6b');
}).toThrowError(
`Invalid second argument, test6b. It must be a callback function.`,
'Invalid second argument, test6b. It must be a callback function.',
);
});
});
10 changes: 10 additions & 0 deletions packages/jest-jasmine2/src/jasmine/Env.js
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,16 @@ export default function(j$) {

this.describe = function(description, specDefinitions) {
const suite = suiteFactory(description);
if (specDefinitions === undefined) {
throw new Error(
`Missing second argument. It must be a callback function.`,
);
}
if (typeof specDefinitions !== 'function') {
throw new Error(
`Invalid second argument, ${specDefinitions}. It must be a callback function.`,
);
}
if (specDefinitions.length > 0) {
throw new Error('describe does not expect any arguments');
}
Expand Down