What Is the Default Coverage Threshold

grant commented 1 year ago

Describe a bug

I'm using this action with a repo setup that doesn't have a coverage threshold. For reference, our coverage is around 10-20%.

We're seeing St.❔ 🔴 for the status result. This is making engineers think something is wrong with PRs.

Questions:

Can we remove this column for jest configs without a coverageThreshold? I wouldn't expect this action tell me if my code is above or below a threshold if I have not defined one in my config.
What is the threshold? It could be in the alt text with St. ?.

I think the code is here:

https://github.com/ArtiomTr/jest-coverage-report-action/blob/952a05968ad9e0b4ab2ab221a8d7d72e621aec51/src/format/summary/formatCoverageSummary.ts#L23

Which defines a default threshold to 60?

https://github.com/ArtiomTr/jest-coverage-report-action/blob/952a05968ad9e0b4ab2ab221a8d7d72e621aec51/src/utils/getStatusOfPercents.ts#L5

I would argue that 60 is not visible to the end user and shouldn't be set. Only the user or jest should be able to set a default config.

Expected behavior

Category	Percentage	Covered / Total
Statements	21.33% (-0% 🔻)	7162/33582
Branches	12.93% (-0% 🔻)	2412/18660
Functions	18.52%	1764/9524
Lines	21.32% (-0% 🔻)	6525/30599

Actual behavior

St.^{:grey_question:}	Category	Percentage	Covered / Total
🔴	Statements	21.33% (-0% 🔻)	7162/33582
🔴	Branches	12.93% (-0% 🔻)	2412/18660
🔴	Functions	18.52%	1764/9524
🔴	Lines	21.32% (-0% 🔻)	6525/30599

Alternatives Considered

We will probably just make our threshold 0 to ensure PRs are 🟢 ok.

grant commented 1 year ago

It looks like this action assumes that the jest config is a json file (ours is not), and so it improperly gets the coverage threshold config value.

https://github.com/ArtiomTr/jest-coverage-report-action/blob/952a05968ad9e0b4ab2ab221a8d7d72e621aec51/src/utils/getNormalThreshold.ts#L10

grant commented 1 year ago

Hi @ArtiomTr , do you know the answer to this?

Would you say that this default config is a bug? It would be nice if the default 0 so action comment aren't all red 🔴 symbolizing an error when there is none.

ArtiomTr commented 1 year ago

Hello @grant :wave:,

Sorry for the late response. That is a quite complicated problem, although it seems simple at first glance. There are several issues, that occurred due to bad action architecture.

Long explanation

## History Initially, this action was created for serving a simple purpose: a better alternative to "just running tests". See, I was thinking that "just running tests" and measuring how "good" a pull request is, looking at the red/green bulb isn't a good way. I saw tools like [Codecov](https://about.codecov.io/), [Coveralls](https://coveralls.io/), etc., but they seem too complicated for such a simple purpose. So, I've started by doing a very simple thing: moving information that you see in a terminal when running jest with default reporters: ![Screenshot of a terminal, with coverage information produced by "jest --coverage" command](https://user-images.githubusercontent.com/44021713/232248577-594b61d4-f63c-4559-befd-f6143a25c981.png) However, that was *quite a lot* of information to show in a single pull request, so I decided to reduce this information and keep only the "All" category. So, that's the reason why action produces these 4 categories in the output. The text output is good, but that's not really human-readable. So, I decided to add these red/yellow/green icons, so the report will look nice & the person will immediately see some feedback. However, using 60% as the threshold wasn't good for everyone. To deal with this issue, I decided to add an option, named `threshold`: https://github.com/ArtiomTr/jest-coverage-report-action/blob/952a05968ad9e0b4ab2ab221a8d7d72e621aec51/action.yml#L16-L18 Also, this option helped people automatically reject pull requests, when coverage drops dramatically. And everything was cool, *not taking into account the fact that this is absolutely wrong and even harmful.* I was not familiar with all jest features, and I didn't know that there is already an option, named [coverageThreshold](https://jestjs.io/docs/configuration#coveragethreshold-object). So, this option introduced a bad habit - when your CI can pass locally, but fail in PR. Also, this creates a small vendor lock-in issue: you're now forced to use this action as if you migrate to another action, it may handle coverage thresholds differently. So, I've fixed coverage threshold checks, with a simple solution: because jest fails when `coverageThreshold` checks do not pass, I've just added graceful handling of these errors. And for the `threshold` option, I've just set up the `coverageThreshold` property correctly: https://github.com/ArtiomTr/jest-coverage-report-action/blob/952a05968ad9e0b4ab2ab221a8d7d72e621aec51/src/utils/getNormalThreshold.ts#L12-L22 ## Now So now, the action handles threshold checks *almost* correctly, but we have an issue with displaying the right colours for the bulbs (actually, the issue for this bug already exists #235). What is the problem with showing the right colour for each row? > It looks like this action assumes that the jest config is a `json` file (ours is not), and so it improperly gets the coverage threshold config value. > > https://github.com/ArtiomTr/jest-coverage-report-action/blob/952a05968ad9e0b4ab2ab221a8d7d72e621aec51/src/utils/getNormalThreshold.ts#L10 *Actually, the action reads jest configuration properly, via the amazing [c12](https://github.com/unjs/c12) configuration loader.* The problem is that the project could have different threshold groups and even no threshold group for `global`. This means, that to show this correctly, action should generate the whole report differently. I'm sure that most users wouldn't be happy if I will break reports completely, in a minor version bump. I could do a major version bump, however, there are more things that I want to break 😄. Jest develops rapidly, and more and more features appear. For instance, here are some features that I want to add in the next major version: 1. Native support for jest shards #286 2. Replace statement/branch/function annotations with line annotations #147 3. Speed up action by caching reports #136 4. Detailed coverage view (probably coming later, when [GitHub Blocks](https://blocks.githubnext.com/) will be stable) 5. And more... I don't think that releasing 5-6 major bumps is a good solution, as people will be just frustrated by updating action every month. Also, this will mean that I would need to maintain 2-3 most recent major versions, which will be a total nightmare for me. ## Can we just add another option? I think that's the most commonly suggested solution to any problem. In most cases, adding a new option is a bad idea. There are several reasons, why it's bad: 1. Adding complexity to the action. *Common, that is a small action, why it needs [13 options](https://github.com/ArtiomTr/jest-coverage-report-action/blob/952a05968ad9e0b4ab2ab221a8d7d72e621aec51/action.yml#L7-L53)?* 2. Maintainance hell. If we simplify, and say that those 13 options can have only 2 different values (on/off), then it's already `2^13=8192` different configurations. Of course, most of them won't conflict, but if someone finds an issue, I need to manually reproduce it, and fix it. Testing actions in GitHub isn't very easy: to perform an e2e test, I need to create the repository, pull request, and trigger action. Taking into account the fact, that there are countless options to create a repository on GitHub (public/private, personal/organisation, organisation privileges, cloud/self-hosting, ssh/https) it quickly becomes impossible to test all these cases. 3. DX degradation - it is always nice when action works right out of the box. Having 13 configuration options is clearly a problem because the user will need to configure something. ## Conclusion So, the real problem with this issue, is that I'm stuck in the loop: `Try to fix bug` -> `Understand, that it will break things` -> `Decide to make make a major version to address this issue` -> `Find a ton of other bugs` -> `Try to fix bug` -> and so on. Of course, this is very funny: I can't change the colour of a single bulb 😄. I hope this explanation will make the reasoning behind the decision to "not address this issue" more clear. And so, I propose a simple solution: I will create a separate branch, with the bulbs removed. Then, you could just use that branch, instead of the version. For instance, your `action.yaml` will look like this: ```yaml name: 'coverage' on: pull_request: branches: - master - main jobs: coverage: runs-on: ubuntu-latest steps: - uses: actions/checkout@v3 # ↓ branch name here - uses: ArtiomTr/jest-coverage-report-action@without-bulbs ``` That is not an ideal solution, as you won't receive any updates. But, this will fix your issue and will have 0 maintenance cost for me.

TLDR

This issue could not be easily fixed due to the structure of the coverageThreshold field in jest.config.js

I can fix that by creating an exclusive version, without those bulbs. It will be just a separate branch in a repository, and you could use it like this:

name: 'coverage'
on:
pull_request:
    branches:
        - master
        - main
jobs:
coverage:
    runs-on: ubuntu-latest
    steps:
        - uses: actions/checkout@v3
                                                    # ↓ branch name here 
        - uses: ArtiomTr/jest-coverage-report-action@without-bulbs

Let me know if this solution will fit your needs.

grant commented 1 year ago

Thanks. I don't think we're going to use this Action for our use-case. For this issue, was just looking for an option to disable the column or use our config.