Rule to guarantee coverage and conf to display warns for not covered

[mgtitimoli]

Hi @amilajack,

Before moving forward I want to say thank you, I'm using your package since the first day I dug into flowtype and it works great!

I don't know if you have ever tried Nuclide, well, the point is that it has a nice feature that lets you see which parts of a given file is not covered by flow. I was wondering since this can also be grouped into linting issues, if it would be a good thing to add a rule called for example enforce-min-coverage (I believe given its name, it should be clear what it does 🙂), and also add support due a configuration value (could be named display-not-covered) to display the sections (eslint warning) of the code that are not currently being covered by flow.

What do you think?

[amilajack]

I have used nuclide before but I don't remember seeing something like a min flow types coverage check. Can you send me a screenshot of that? That might help me understand this more. I also think that enforce-min-coverage should be a separate tool. eslint is primarily used for reporting errors in specific lines of code. I think a better solution would add the flow-coverage-report checker in your lint script:

{
  "scripts": {
    "lint": "flow-coverage-report --min-coverage=50% && eslint ."
  }
}

Thoughts on this?

[mgtitimoli]

Sure, here is a screenshot so you can see what I'm talking about

As you can see, they don't have a min flow types coverage, what they do have is a feature that on hover shows a tooltip where they tell you the percentage covered by flow, and in addition if you click on it, then it toggles a feature that underlines all the code sections that are not covered.

The idea would be to replicate this behavior, and control the error level used with display-not-covered rule, I know in my previous post I wrote that it should be a conf value, but now that I think it better, it could also be a rule and use its level to control how all the parts that are not covered by flow are shown (as errors, warnings, or disable them).

In addition to this and going back to the other idea I mentioned (the one about enforce-min-coverage), one alternative would be to use Program node type, and run the correspondent flow command to get the coverage analysis and then do some math to get the percentage and contrast it with the value specified in the rule config, and in case it's below the specified value use the context parameter received in node visitor function, to rise the proper feedback, error/warning, so at the end you would run this rule once per file (Program node).

I hope I have explained better what I think.

[mgtitimoli]

For enforce-min-coverage would be something like this:

module.exports = {
    meta: { ... },
    create: context => ({
       Program: node => {
           // 1. get flow coverage
           // 2. do some math to calculate percentage
           // 3. place it in a const like for example actualCovered :)
           const requiredCoverage = context.options[0];
           if (actualCovered < requiredCoverage) {
               context.report({
                   node: node,
                   message:
                       `Expected coverage to be at least ${requiredCoverage}, ` +
                       `but is: ${actualCovered}`
               });
           }
       }
    })
};

One thing that could be hard to know is if a given file has flow annotation, but I guess you are already doing that, if not, it would be a matter of visiting Comment nodes, and look for flow annotation, and change Program by Program:exit.

[amilajack]

I see. How about making a PR for this? The code base is very small so its pretty easy to navigate. Also I'm a bit busy at the moment so it may take a while before I get around to looking over and doing this. Thanks!

[jzimmek]

Hi @amilajack,

I put together a enforce-min-coverage rule:
https://github.com/jzimmek/eslint-plugin-flowtype-errors

Not sure how you want this to be implemented into the existing codebase, so I tried to modify a minimum of existing code. Some code could be further unified.

I am still struggling with the testing infrastructure. Seems like it is tightly focused on the collect functionality - which made total sense till now ;-)

The enforce-min-coverage rule takes the required coverage (as percentage) as an option:

...
  rules:
    strict: 0
    flowtype-errors/show-errors: 2
    flowtype-errors/enforce-min-coverage:
      - 2
      - 50
...

Please let me know what I should change / enhance so you would consider a pull request.

[amilajack]

@jzimmek would be awesome if you could PR this! 🎉

[jzimmek]

#84

[mgtitimoli]

Nice job @jzimmek!

Correct me if I'm wrong, but I believe there are two little details missed on the PR

1. Add a section in the README showing how to use the rule
2. There is a bug here (shouldn't be stdout = childProcess.spawnSync(getFlowBin(), ['coverage', '--json']).stdout;?)

Thanks again for taking the time to do this 🙂

[jzimmek]

Thanks @mgtitimoli.

Will add a PR for the readme today. Meanwhile you can use it this way:

yarn add eslint-plugin-flowtype-errors@beta

add the following rule (enforce 50% coverage) to your .eslintrc:

flowtype-errors/enforce-min-coverage: [2, 50]

You are somehow right about 2. It is not a bug, because the default case will never be entered. The coverage function is always file based as of now. I copy/pasted this code part from the existing collect function which can also be invoked on a global level without any specific file. Will clean this up.