azeemba / eslint-plugin-json

Lint your JSON files
MIT License
209 stars 29 forks source link

Add unit tests for rules to bring up test coverage #88

Closed BePo65 closed 4 months ago

BePo65 commented 5 months ago

After pr #87 (or even before) test coverage was quite low, as only the processor was tested.

So I copied parts of the integration tests to bring up test coverage. These tests are quite slow (but there are only 9 tests). Unluckily I was not able to find a configuration for the eslint ruleTester that accepted a processor option. So I stayed with the code from the integration test.

Perhaps things will get better, if the ides from issue #85 will be implemented.

azeemba commented 5 months ago

Thanks for the PR!

If our goal is to increase the test coverage, I wonder if there is a way to report the coverage from our integration tests instead? I worry about duplicating the code across the unit tests and the integration tests.

BePo65 commented 5 months ago

I know that my code is clumsy, but I wanted to change the existing code as little as possible.

So perhaps we could / should do:

  1. get coverage from the integration tests and send them to codecov (I would use 1 coverage report per version of eslint used)
  2. as we still need different configuration files for flat and legacy eslint configurations, extract the common methods for the integration tests into a separate file and thus remove duplicated code.
  3. drop the rules tests from the unit-test.

Probably the problem will be that the coverage of all the single tests will be less than 100%, even when in the end all lines / branches were tested. I'll try to find a solution for this.

Anyway I will start with points 1. and 2. or what do you think.

azeemba commented 5 months ago

Thanks for the update!

I appreciate the goal of minimizing size of the PRs. In this case, since there is no time sensitivity or negative user impact, I prefer doing a more final solution. To be honest, I don't know what the final solution should be though.

I am pretty happy with our CI that runs the integration tests and gives us pretty good confidence in our code base. So, however we choose to improve the test coverage, it should not compromise the quality of the integration tests or make the integration tests harder to maintain. I am happy with any solution that avoids that.

BePo65 commented 5 months ago

I'll do my best 😄

BePo65 commented 4 months ago

OK, let's forget about this pr.

It was a good idea, but I did not find a way to get coverage out of the rule tests.

So in the end I learned a lot about eslint plugins and perhaps this was worth it all 😃