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

textlint-tester: use with plugin #488

Closed
0x6b opened this issue Jan 21, 2018 · 9 comments

Comments

@0x6b
Copy link
Member

commented Jan 21, 2018

What version of textlint are you using?

What file type (Markdown, plain text, etc.) are you using?

HTML

What did you do? Please include the actual source code causing the issue.

  1. Have been updating dependencies for textlint/examples/html-plugin for housekeeping
  2. Updated textlint-rule-sentence-length from 1.0.4 to 2.0.0
  3. Found that sentence-splitter 3.0.6 (which textlint-rule-sentence-length 2.0.0 depends on) has a defect which causes infinite loop and out of memory error (azu/sentence-splitter#9)
  4. @azu kindly released 3.0.7 which solved the issue
  5. Back to textlint-rule-sentence-length, updated sentence-splitter to 3.0.7
  6. Added valid test case to textlint-rule-sentence-length.js as follows:
{
    text: "<p>this is a test</p>",
    ext: ".html"
}

What did you expect to happen?

Test finished without error expecting textlint uses html plugin according to ext property described at README.

What actually happened?

Got following error:

  1) textlint-rule-sentence-length
       <p>this is a test</p>:
     Error: Not found available plugin for .html
      at node_modules/@textlint/kernel/src/textlint-kernel.ts:116:23
      at <anonymous>

Reviewed textlint-tester.ts and found that the tester does not initialize plugins according to ext property as expected.

const textlint = new TextLintCore();
textlint.setupRules(

Simply adding pluginName and plugin parameters then call setupPlugins as follows solved the issue, but function signature looks ugly for me.

  • In order to maintain compatibility, I think pluginName and plugin parameters should be optional, but
  • A required parameter cannot follow an optional parameter (TS1016)
--- a/packages/textlint-tester/src/textlint-tester.ts
+++ b/packages/textlint-tester/src/textlint-tester.ts
@@ -71,12 +71,28 @@ export class TextLintTester {
         }
     }

-    testValidPattern(ruleName: string, rule: TextlintRuleCreator, valid: TesterValid) {
+    testValidPattern(ruleName: string, rule: TextlintRuleCreator, valid: TesterValid,
+        pluginName?: string,
+        plugin?: object
+    ) {
         const text = typeof valid === "object" ? valid.text : valid;
         const inputPath = typeof valid === "object" ? valid.inputPath : undefined;
         const options = (typeof valid === "object" && valid.options) || {};
         const ext = typeof valid === "object" && valid.ext !== undefined ? valid.ext : ".md";
         const textlint = new TextLintCore();
+        if (pluginName && plugin) {
+            textlint.setupPlugins({ [pluginName]: plugin }, { [pluginName]: options });
+        }
         textlint.setupRules(
             {
                 [ruleName]: rule

(omit rest for brevity)

Test script:

import TextLintTester = require("../src/index");
const noTodo = require("textlint-rule-no-todo");
const htmlPlugin = require("textlint-plugin-html");
const tester = new TextLintTester();
tester.run(
    "no-todo", noTodo, { valid: [...], invalid: [...] },
    "textlint-plugin-html", htmlPlugin // new parameters
);

Possible options I can think of is:

  1. Add plugin related parameters as above
  2. Use textlint-engine-core from textlint-tester (a lot of work?)
  3. Leave current implementation as is, and document this behavior—can't test with plugins other than default text and markdown—as limitation
  4. Any other

Appreciate your input. Thank you.

@azu

This comment has been minimized.

Copy link
Member

commented Jan 22, 2018

Af frist, textlint-tester aim to test textlint rule.
This is inspired by ESLint RuleTester.

Main usecase of textlint-tester is just test a single textlint rule.
But, We have got other usecase like this issue.

Current Limitation

Currently, textlint-tester can not does following:

  • Test multiple rules at once
    • Test conbination of some rules
  • Test plugin processor
    • In this issue
  • Test filter rule

Proposals

We have five+ options for extensing testing.

  1. TextLintTester constructor options
  • new TextLintTester({ plugins: [ ... ] })
  1. TextLintTester#setupPlugin() method (#342 idea)
  2. Addtional a parameter to run method ( @0x6b suggestion)
  3. Overload rule paramerter of run method
  • accept rule, or plugin, filter as run parameter
- run(ruleName: string, rule: TextlintRuleCreator, {valid, invalid}: {
+ run(testName: string, testTarget: TestTarget, {valid, invalid}: {
        valid?: TesterValid[];
        invalid?: TesterInvalid[];
    }): void;
// kernel-like
interface TestTarget {
      plugins: [{ pluginId: string, plugin: TextlintPluginCreator }],
      rules: [{ ruleId: string, rule: TextlintRuleCreator }] 
}
  1. valid or invalid options
  • Align with .ext option

1 ~ 4 proposal is very similar purpose, but these have different API.

  • 1~2 extends TextLintTester instance
  • 3~4 extends run option

5 is quite different life cyle with other proposal.
5 can change the plugin per test case.

@azu

This comment has been minimized.

Copy link
Member

commented Jan 22, 2018

My opinion for each proposal.

1.TextLintTester constructor options

and

  1. TextLintTester#setupPlugin() method (#342 idea)

textlint-tester have already accepected rule as run paramaeter.
1 and 2 add plugin parameter to different points.
I think that it make the user confuse.
The user should extends difference points for testing plugin.

  • How to test plugin?
    • => use constructor option /method
  • How to test rule?
    • => use run option
  1. Addtional a parameter to run method ( @0x6b suggestion)

It it diffcult that extend another features in the future.
If we support filter rule, we need to get more parameter.

tester.run(name, rule, { valid, invalid }, plugin, filter)
  1. Overload rule paramerter of run method

It can meet a requirement.

  • can test multiple rules
  • can work with plugin
  • can test filter rules
import TextLintTester = require("../src/index");
const noTodo = require("textlint-rule-no-todo");
const htmlPlugin = require("textlint-plugin-html");
const tester = new TextLintTester();
tester.run(
    "no-todo with html", {
      "rules": [{
         ruleId: "no-todo":
         rule: noTodo
      }],
      "plugins": [{
         pluginId: "html",
         plugin: htmlPlugin
      }],
    }, { valid: [...], invalid: [...] }
);

📝 Probabaly, It can get backword compatible.

import TextLintTester = require("../src/index");
const noTodo = require("textlint-rule-no-todo");
tester.run(
    "no-todo", noTodo, { valid: [...], invalid: [...] }
);

is just shortcut of

import TextLintTester = require("../src/index");
const noTodo = require("textlint-rule-no-todo");
tester.run(
    "no-todo", { rules:[{ ruleId: "no-todo", rule: noTodo }] }: , { valid: [...], invalid: [...] }
);
  1. valid or invalid options

I have no idea.
It is best flexible, but the API interface is difficult.

@textlint/kernel has similar concept that lint text with all options.
But, run method's second arguments(rule) conflict with that options.

import TextLintTester = require("../src/index");
const noTodo = require("textlint-rule-no-todo");
tester.run(
    "no-todo", rule , { valid: [
{
  text: "test",
  ext: ".ext",
  plugins: [ htmlPlugin ]
  filters: [...]
  (additional)rules?: [...]
}
], invalid: [...] }
);

FYI: ESLint's RuleTester constructor option is base option between each test cases.

The RuleTester constructor accepts an optional object argument, which can be used to specify defaults for your test cases. For example, if all of your test cases use ES2015, you can set it as a default:
-- https://eslint.org/docs/developer-guide/nodejs-api#ruletester

@0x6b

This comment has been minimized.

Copy link
Member Author

commented Jan 22, 2018

@azu, thank you very much for your extensive comments. I think option 4 is much reasonable as it can provide backward compatibility. To move this discussion forward, I have implemented proof of concept at this branch (diff) so could you have a look into it? This implementation:

  • can test multiple rules
  • can work with multiple plugins
  • can test filter rules (but not implemented so far)
  • provides backward compatibility

Further discussion points:

  • Does supporting multiple plugins make sense? "plugins": [{ pluginId: "html", plugin: htmlPlugin }] (#488 (comment)) could be "plugin": { pluginId: "html", plugin: htmlPlugin }.
  • Is this overloading implementation idiomatic from textlint and/or TypeScript point of view?
  • Any other idea or comment?

Let me know if you need WIP pull request for further review. Thanks!

@0x6b

This comment has been minimized.

Copy link
Member Author

commented Jan 22, 2018

Postscript: I'm sorry I have failed to find #342 and open duplicate issue 🤔

@azu

This comment has been minimized.

Copy link
Member

commented Jan 22, 2018

I have implemented proof of concept at this branch (diff) so could you have a look into it?

Thanks for work!

Does supporting multiple plugins make sense?

I've written plugins without thinking so much.
If we can support rules(plural form) option in TestTarget, we should support plugins(plural form).
It pull the whole look together.

Of course, we can support plugin and plugins.
But, the implementation cost is larger than plugins only.

Is this overloading implementation idiomatic from textlint and/or TypeScript point of view?

TypeScript has Type Guards feature.
It will remove (param as TestTarget) cast.

- function isTestTarget(arg: any) {
+ function isTestTarget(arg: any): arg is TestTarget {
  • param: TextlintRuleCreator | TestTarget
  • if isTestTarget(param)
    • param is TestTarget
  • else
    • param is (TextlintRuleCreator | TestTarget) - TestTarget
    • As as result, param is TextlintRuleCreator

Any other idea or comment?

We need to get good name for TestTarget :-)

We want to get Integration test for checking backward compatible.

@0x6b

This comment has been minimized.

Copy link
Member Author

commented Jan 22, 2018

@azu, thanks much for quick review.

  • Multiple plugins: singular and plural is difficult for us Japanese... Go with plugins
  • Type Guards: looks at first difficult to understand but really smart. Implemented at 0x6b@67b006d
  • Good name for TestTarget: based on existing implementation, TestConfig?

export class Config {

  • Integration test: I'm not sure what it should be, but something like eslint/rule-tester?
@azu

This comment has been minimized.

Copy link
Member

commented Jan 22, 2018

Ok. welcome to open PR 🚪

Integration test: I'm not sure what it should be, but something like eslint/rule-tester?

textlint-tester version of https://github.com/textlint/textlint/tree/master/test/integration-test.
Use real code for testing.

@0x6b

This comment has been minimized.

Copy link
Member Author

commented Jan 22, 2018

@azu, updated branch is under testing on Travis CI. Will open a pull request. Thank you very much for your review. As for integration test, I got your point. I'll take it a look. Thank you for clarification.

@azu

This comment has been minimized.

Copy link
Member

commented Jan 25, 2018

We have added testConfig parameter into run method.
It support that multiple rules and plugins.

For more details, see textlint-tester.

0x6b added a commit to 0x6b/textlint-rule-sentence-length that referenced this issue Jan 27, 2018
`textlint-tester` 4.0.6, which `textlint-scripts` 1.4.0 depends on,
did not support test with plugin (textlint/textlint#488) so it was
impossible to add test case. `textlint-tester` 4.1.0 added support
for use with plugin (textlint/textlint#493) so I can improve test
coverage.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.