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

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

Closed
wants to merge 2 commits into from

Conversation

BePo65
Copy link
Contributor

@BePo65 BePo65 commented May 29, 2024

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
Copy link
Owner

azeemba commented Jun 1, 2024

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
Copy link
Contributor Author

BePo65 commented Jun 6, 2024

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
Copy link
Owner

azeemba commented Jun 6, 2024

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
Copy link
Contributor Author

BePo65 commented Jun 6, 2024

I'll do my best 😄

@BePo65
Copy link
Contributor Author

BePo65 commented Jun 8, 2024

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.

  • The way I tried it in the code of this pr does not work, as for testing the rules (and in the integration tests) eslint runs in a separate process and I did not find a way to get coverage for this. I do not know, why this seemed to work when I wrote this pr, but by now, 'unit.test.js' does not show coverage for the rules :-(
  • I even tried the eslint ruleTester, but it does not handle 'processor' configuration settings.
  • And using eslint in code has the same problem as my original approach.

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

@BePo65 BePo65 closed this Jun 8, 2024
@BePo65 BePo65 deleted the add-rules-unit-tests branch June 9, 2024 11: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.

2 participants