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

Docs: Add section about customizing RuleTester (fixes #6227) #6331

Merged
merged 3 commits into from Jun 9, 2016

Conversation

jfmengels
Copy link
Contributor

Following the discussion we had in (#6227), I'm adding a section about how to customize RuleTester to work with different test runners like AVA. Didn't go into too much detail, as I figure you'll probably need to go and read the code anyway. At least people will now know that it's possible.

Let me know if you have suggestions on how to improve this.

(PS: If you found this by searching for AVA, I wrote the following package to allow RuleTester to be used with AVA https://github.com/jfmengels/eslint-ava-rule-tester)

@eslintbot
Copy link

LGTM

@ilyavolodin
Copy link
Member

LGTM. Thanks

@@ -140,6 +140,11 @@ ruleTester.run("custom-plugin-rule", rule, {
});
```

#### Customizing RuleTester

RuleTester internally uses Mocha's `describe` and `it` methods when available to create tests for each valid and invalid case. If you use another test runner, you can override `RuleTester.describe` and `RuleTester.it` to make RuleTester compatible with it and have proper individual tests and feedback.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would link to https://github.com/jfmengels/eslint-ava-rule-tester as an example on how to do it.

Copy link
Member

Choose a reason for hiding this comment

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

Super content: says much in few words. Here are a few suggestions about grammar of the first sentence:

  • move to beginning: To create tests for each valid and invalid case,
  • is internally implied in this context?
  • avoid possessive on product name and include its tag line: RuleTester uses describe and it methods from the Mocha test framework when it is available.

Copy link
Contributor Author

@jfmengels jfmengels Jun 8, 2016

Choose a reason for hiding this comment

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

Point 1 and 3 sound good.

Not sure I get point 2 though. Are you saying I should leave out the word internally as it may already be implied? I don't think it hurts to make this explicit.

What about @sindresorhus's comment above showing an example? I think having an example is nice, and my package is working well (in "production" in 2 plugins already), but I don't get the feeling that there are a lot of links to external packages that are not in the ESLint org (and thus out of your control?).

Copy link
Member

Choose a reason for hiding this comment

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

Yes, if with “internally” reads better to you as an example of audience for this section, keep it.

Your observation about the link agrees with what I have seen.

Copy link
Member

Choose a reason for hiding this comment

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

For what it's worth, I (as a user) would prefer "internally" to be explicit here. But I also like a lot of words to be explicit, possibly more than the average developer. Without "internally", I might assume a stronger dependency link (until I get to "when available" six words later).

@eslintbot
Copy link

LGTM

@jfmengels
Copy link
Contributor Author

Updated with the feedback in #6331 (comment). I kept the word internally. Any thoughts on adding a link to https://github.com/jfmengels/eslint-ava-rule-tester as an example as suggested by @sindresorhus? As it is, I'll leave it out, but it can be nice to have as it completes the documentation.

@@ -140,6 +140,11 @@ ruleTester.run("custom-plugin-rule", rule, {
});
```

#### Customizing RuleTester

To create tests for each valid and invalid case, RuleTester internally uses `describe` and `it` methods from the Mocha test framework when it is available. If you use another test framework, you can override `RuleTester.describe` and `RuleTester.it` to make RuleTester compatible with it and have proper individual tests and feedback.
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 make RuleTester in code front everywhere? Seems best to keep it consistent.

Also, maybe show an example right here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. when you say code front, you mean like this, right?

@nzakas
Copy link
Member

nzakas commented Jun 8, 2016

I don't think linking to the AVA rule tester is all that valuable in this context. I'd just throw in an simple example right under the paragraph and call it done.

@eslintbot
Copy link

LGTM

@jfmengels
Copy link
Contributor Author

I've added "`" around RuleTester, and added an example. Let me know what you think.

@platinumazure
Copy link
Member

LGTM. One question (and this isn't even about a possible flaw with the PR): When/how would code that overrides RuleTester.describe and RuleTester.it need to be run? Once per test run? In a snippet included in every rule file? What's the best strategy for that sort of thing?

And once I have that answer, maybe we'll know if that needs to be added to the docs 😄

@jfmengels
Copy link
Contributor Author

@platinumazure It would need to be run once per test run (by that I mean the whole test suite run), before any test declaration. Just to be sure that it will be done on every test run even if you run a single test file, I'd recommend doing this once at the top of each test file (as you can re-override it multiple times without problem).

With my adapter, here you have an [example](https://github.com/avajs/eslint-plugin-ava/blob/master/test/use-t.js. Its not the exact same as in the example, as it is more practical for several reasons to pass the test creator to the adapter, but it's really close. The example should get the message accross or work for simple not-DRY tests.

@ilyavolodin
Copy link
Member

LGTM. Thanks!

@platinumazure
Copy link
Member

One more dumb question (don't let this stop a merge): I assume we definitely do not want to invite people to switch the test runner for core ESLint development, and therefore this should definitely not be included in working-with-rules or other non-plugin docs?

@jfmengels
Copy link
Contributor Author

I assume we definitely do not want to invite people to switch the test runner for core ESLint development

Would agree to that. I don't think that the wording pushes you to switch. That said, I think it's better to add an extra sentence to tell the user they don't need to do that for core ESLint rules, if you deem it necessary, than to move this elsewhere (as it's the only place that mentions RuleTester).

@platinumazure
Copy link
Member

platinumazure commented Jun 9, 2016

@jfmengels Nah, I don't think this needs an edit-- I was asking for my own education 😄

LGTM.

@nzakas
Copy link
Member

nzakas commented Jun 9, 2016

This looks great. Thanks @jfmengels!

@nzakas nzakas merged commit 477fbc1 into eslint:master Jun 9, 2016
@jfmengels
Copy link
Contributor Author

No problem, happy to help. And glad to know that I can "hack" TestRunner, our tests and feedback are way better now :)

@jfmengels jfmengels deleted the issue-6227 branch June 10, 2016 15:00
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants