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

Add test.todo #6996

Merged
merged 11 commits into from
Sep 29, 2018
Merged

Add test.todo #6996

merged 11 commits into from
Sep 29, 2018

Conversation

mattphillips
Copy link
Contributor

@mattphillips mattphillips commented Sep 18, 2018

Summary

Adds test.todo and it.todo similar to ava's todo. I'm opening this up early for feedback from @SimenB @thymikee @rickhanlonii @aaronabramov 😄

Motivation: see comment by @SimenB

Closes #6430 #6602 and addresses #6256

This change re-introduces the behaviour of planning a test, similar to Jest@22 test('do something')

Test plan

  • unit tests ✅
  • e2e tests ✅

Given this test suite:

it('passes', () => {
  expect(10).toBe(10);
});

it('fails', () => {
  expect(10).toBe(101);
});

it.skip('skips', () => {
  expect(10).toBe(101);
});

it.todo('todo');

The new output will look like:

screen shot 2018-09-18 at 20 54 02

Todo

  • Add docs
  • Add typings

@arcdev1
Copy link

arcdev1 commented Sep 18, 2018

If this gets merged, it would be nice to add a configuration option that allows for treating missing callbacks as "todos" so that we support the legacy behaviour for those who want/need it. That would be a great way to fully address: #6602 #6430 and #6256 in my opinion.

@thymikee
Copy link
Collaborator

This is lovely!

@pk-nb
Copy link

pk-nb commented Sep 18, 2018

Thanks for doing this! I love the new state in the output, regardless of the function option. If this is merged, it would be good to add this to the jest-codemods so that pending specs in jasmine/mocha get converted.

Copy link
Collaborator

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

So nice

"FAIL __tests__/todo_non_string.test.js
● Test suite failed to run

Invalid first argument: () => {}. Todo must be called with a string.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This might look weird, when the fn is not a oneliner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, perhaps we should just remove this as per @rickhanlonii's comments below. I'm pretty flexible with how this API looks. Maybe just validation of a minimum one arg with the first being a string? :)

Copy link
Member

Choose a reason for hiding this comment

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

I think just a simple "todo must be called with just a string", or something similar, is better. No need to stringify the implementation as long as our stack trace point points back to the declaration

| ^
9 |

at packages/jest-jasmine2/build/jasmine/Env.js:480:15
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we strip this from stack trace? cc @SimenB

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SimenB please teach me this magic (again), I've tried but not been able to remove it :(

@@ -88,6 +88,10 @@ exports.interface = function(jasmine: Jasmine, env: any) {
return env.xit.apply(env, arguments);
},

tit(description: string) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

lol

Copy link
Contributor Author

Choose a reason for hiding this comment

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

haha this was my first attempt at getting it to work - I thought I removed it 🤣

Copy link
Member

@rickhanlonii rickhanlonii left a comment

Choose a reason for hiding this comment

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

This is awesome

What do you think about allowing and ignoring the function argument?

I'm thinking of two use cases:

  • Stubbing out future tests to finish later
  • Making it easy to "disable" tests with a todo to come back and fix them

@SimenB
Copy link
Member

SimenB commented Sep 18, 2018

@skovhus for the codemod ❤️

@SimenB
Copy link
Member

SimenB commented Sep 18, 2018

If this gets merged, it would be nice to add a configuration option that allows for treating missing callbacks as "todos" so that we support the legacy behaviour for those who want/need it.

I don't think it's worth it. That's behaviour from almost 6 months ago, and I think a codemod fulfills the same purpose better.

I'm thinking of two use cases:

Aren't those covered by .skip?

@rickhanlonii
Copy link
Member

Aren't those covered by .skip?

Does that depend on the use case? What's really the difference between .skip and .todo?

@arcdev1
Copy link

arcdev1 commented Sep 18, 2018

What's really the difference between .skip and .todo?

todo implies there is work to be done.
skip implies we want to ignore the test for now.

Generally we don't want to ignore work to be done.

I think the first use case; stubbing for later is a valid todo. The second one sounds like skip to me.

@@ -121,6 +121,26 @@ test.only = (testName: TestName, fn: TestFn, timeout?: number) => {
});
};

test.todo = (testName: TestName, ...rest: Array<mixed>) => {
if (rest.length > 0 || typeof testName !== 'string') {
throw new Error('Todo must be called with only a description.');
Copy link
Member

Choose a reason for hiding this comment

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

const e = new Error('Todo must be called with only a description.');

if (Error.captureStackTrace) {
  Error.captureStackTrace(e, test.todo);
}

throw e;

ish 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

❤️amazing!

I tried something similar to this but wasn't passing the correct call site

@SimenB
Copy link
Member

SimenB commented Sep 19, 2018

We could ignore the implementation and have a lint rule for it. I do like that it throws though, so you don't forget it by accident

@SimenB
Copy link
Member

SimenB commented Sep 19, 2018

Also missing changelog 😱

@codecov-io
Copy link

Codecov Report

Merging #6996 into master will decrease coverage by 0.21%.
The diff coverage is 15.51%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6996      +/-   ##
==========================================
- Coverage    66.9%   66.68%   -0.22%     
==========================================
  Files         250      250              
  Lines       10436    10483      +47     
  Branches        3        3              
==========================================
+ Hits         6982     6991       +9     
- Misses       3453     3491      +38     
  Partials        1        1
Impacted Files Coverage Δ
packages/jest-cli/src/constants.js 100% <ø> (ø) ⬆️
packages/jest-jasmine2/src/jasmine/Env.js 0% <0%> (ø) ⬆️
packages/jest-jasmine2/src/index.js 0% <0%> (ø) ⬆️
.../src/legacy_code_todo_rewrite/jest_adapter_init.js 0% <0%> (ø) ⬆️
packages/jest-runner/src/run_test.js 2.98% <0%> (ø) ⬆️
...ackages/jest-cli/src/reporters/verbose_reporter.js 20.37% <0%> (-2.08%) ⬇️
packages/jest-jasmine2/src/jasmine/Spec.js 0% <0%> (ø) ⬆️
packages/jest-circus/src/run.js 0% <0%> (ø) ⬆️
packages/jest-circus/src/event_handler.js 12% <0%> (-0.33%) ⬇️
packages/jest-cli/src/testResultHelpers.js 9.09% <0%> (-0.29%) ⬇️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2af1c62...eb2289c. Read the comment docs.

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

CHANGELOG.md Outdated
@@ -2,6 +2,8 @@

### Features

- `[jest-jasmine2/jest-circus/jest-cli]` Add test.todo ([#6996](https://github.com/facebook/jest/pull/6996))

Copy link
Member

Choose a reason for hiding this comment

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

remove this newline

expect(result.status).toBe(1);
const output = extractSummary(result.stderr)
.rest.split('\n')
.map(line => line.trimRight())
Copy link
Member

Choose a reason for hiding this comment

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

Is this needed? If yes, please extract to a helper function instead of copying it in every test

@mattphillips
Copy link
Contributor Author

ping @SimenB just updated with your comments :)

@SimenB
Copy link
Member

SimenB commented Sep 21, 2018

@cpojer @aaronabramov @mjesun this is new API, so I'd like your approval before merging 🙂

@palmerj3
Copy link
Contributor

I like seeing todo's aggregated separately from skipped tests for reporters. Nice work!

@SimenB
Copy link
Member

SimenB commented Sep 24, 2018

@mattphillips could you update the error we throw when it or test is missing an implementation to suggest using this new method?

@@ -15,6 +15,7 @@ export const ICONS = {
failed: isWindows ? '\u00D7' : '\u2715',
pending: '\u25CB',
success: isWindows ? '\u221A' : '\u2713',
todo: '\u270E',
Copy link
Member

Choose a reason for hiding this comment

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

Can you check how this looks on windows?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SimenB I don't have a windows machine, does anyone else have one?

Copy link
Member

Choose a reason for hiding this comment

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

@wldcordeiro
Copy link

Adding a --todo flag that made a little table of the todos like --coverage does would be icing on the cake! I love this! 🎉 😄

@SimenB
Copy link
Member

SimenB commented Sep 25, 2018

Once we land this you can do that with a custom reporter 🙂

@rickhanlonii
Copy link
Member

jest-reporter-todo would be dope

@SimenB
Copy link
Member

SimenB commented Sep 29, 2018

@mattphillips ping 🙂 We can iterate on the name after landing if we want

@SimenB
Copy link
Member

SimenB commented Sep 29, 2018

image

@github-actions
Copy link

This pull request 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Excluding second argument should not throw, but instead mark the test as skipped
10 participants