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

Consistent-test-it rule #64

Merged
merged 9 commits into from
Feb 11, 2018
Merged

Consistent-test-it rule #64

merged 9 commits into from
Feb 11, 2018

Conversation

ranyitz
Copy link
Contributor

@ranyitz ranyitz commented Feb 10, 2018

Following the discussion in #12

Summary

This rule gives you control over the usage of these keywords in your codebase.
You can control which style you prefer, within describe or outside.

  • {"fn": "it"} enforce it calls and fix test to be it.
  • {"fn": "test"} enfore test calls and fix it to be test.
  • {"fn": "test", "withinDescribe":"it"} enforce test on top level tests and enforce it when nested inside of describe, fix it to test when using in top level, fix test to it when using inside of describe.
  • add documentation.

README.md Outdated
@@ -80,6 +80,7 @@ for more information about extending configuration files.

| Rule | Description | Recommended | Fixable |
| ------------------------------------------------------------------ | --------------------------------------------------------------- | ----------------------------------------------------------------------- | ----------------------------------------------------------- |
| [consistent-test-it](docs/rules/consistent-test-it.md) | Enforce consistent test or it keyword | ![recommended](https://img.shields.io/badge/-recommended-lightgrey.svg) | ![fixable](https://img.shields.io/badge/-fixable-green.svg) |
Copy link
Member

Choose a reason for hiding this comment

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

Not recommended

Copy link
Contributor Author

Choose a reason for hiding this comment

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

badge removed


context.report({
message:
"Prefer using '{{ testKeyword }}' on '{{ opositeTestKeyword }}'",
Copy link
Member

Choose a reason for hiding this comment

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

Switch "on" with "instead of" perhaps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, "instead of" is better.

@SimenB SimenB requested a review from thymikee February 10, 2018 23:10
@@ -0,0 +1,124 @@
'use strict';

const { getNodeName, isTestCase, isDescribe } = require('./util');
Copy link
Member

Choose a reason for hiding this comment

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

cannot use destructuring due to node 4 support - see failing CI

Copy link
Collaborator

Choose a reason for hiding this comment

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

May be worth adding the es5/no-destructuring rule to help prevent/communicate this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Opened #66 to address this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I see, Will be fixed

Copy link
Collaborator

@macklinu macklinu left a comment

Choose a reason for hiding this comment

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

A few minor things/typos, but looking like it covers all the desired cases. Nice work, thanks for submitting a PR!

],
},
create(context) {
const configObj = context.options[0];
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you could declare this like:

const configObj = context.options[0] || {}

And then you're always dealing with a truthy object, so that on the following lines, you do not need to call configObj && configObj.propertyName.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree about that. Thanks

describeNestingLevel === 0 &&
nodeName.indexOf(testKeyword) === -1
) {
const opositeTestKeyword = getOpositeTestKeyword(testKeyword);
Copy link
Collaborator

Choose a reason for hiding this comment

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

oposite is misspelled - should be opposite.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🙃

},
};

function getPreferedNodeName(nodeName, preferedTestKeyword) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

prefered is mispelled - should be preferred.

@ranyitz
Copy link
Contributor Author

ranyitz commented Feb 11, 2018

@macklinu @SimenB Thanks for the review! 😃
I made the fixes according to comments.

Copy link
Collaborator

@macklinu macklinu left a comment

Choose a reason for hiding this comment

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

@ranyitz thanks for addressing those comments. LGTM 😄

const testKeyword = configObj.fn || 'test';
const testKeywordWithinDescribe =
configObj.withinDescribe || configObj.fn || 'it';

Copy link
Member

Choose a reason for hiding this comment

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

This top part is missing some coverage, mind adding the necessary tests to cover all cases?

image

valid: [
{
code: 'test("foo")',
options: [{ fn: 'test', withinDescribe: 'it' }],
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 removing options here is enough to get to 100% coverage 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are right :) will push fix soon

@ranyitz
Copy link
Contributor Author

ranyitz commented Feb 11, 2018

@SimenB Thanks for the comment.
Now We've got 100% coverage here.
I also suggested to add codecov integration which could probably help.

@SimenB SimenB merged commit eb0a2a2 into jest-community:master Feb 11, 2018
@ranyitz ranyitz deleted the rule/always-test-or-it branch February 11, 2018 20:51
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.

None yet

3 participants