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

fix: catch non-numeric min_coverage input #255

Closed
maxzod opened this issue Jun 16, 2023 · 6 comments
Closed

fix: catch non-numeric min_coverage input #255

maxzod opened this issue Jun 16, 2023 · 6 comments
Assignees
Labels
good first issue Good for newcomers

Comments

@maxzod
Copy link

maxzod commented Jun 16, 2023

Describe the bug
current coverage is > 50%
in this workflow

      - name: test coverage
        uses: VeryGoodOpenSource/very_good_coverage@v2
        with:
          min_coverage: 10

it should pass and it does
but in this flow

      - name: test coverage
        uses: VeryGoodOpenSource/very_good_coverage@v2
        with:
          min_coverage: 10%

note: I added the percentage symbol to min_coverage

the result is falling

54.85714285714286 is less than min_coverage 10%

Lines not covered:
  - lib/helpers/http_client.dart: 40, 41, 74
  - lib/generated/tr.dart: 2, 6, 10, 14, 18, 22, 26, 30, 34, 38, 42, 46, 50, 54, 58, 63, 68, 73, 77, 81, 85, 89, 93, 97, 101, 105, 108, 112, 116, 120, 124, 128, 132, 136, 140, 144, 148, 156, 160, 164, 168, 172, 176, 180, 184, 188, 192, 196, 200, 204, 208, 212, 216, 220, 224, 228, 232, 236, 240, 244, 248, 252, 256, 260, 264, 268, 272, 276, 280, 284, 288, 292, 296, 300, 304, 308, 311, 315, 319, 323, 327, 331, 335, 339, 343, 347, 351, 355, 359, 363, 367, 371, 375, 383, 387, 391, 395, 399, 403, 407, 411, 415, 419, 423, 427, 431, 435, 439, 443, 447, 451, 455, 458, 470, 474, 478, 482, 486, 490, 494, 498, 502, 506, 510, 514, 518, 522, 526, 531, 536, 540, 544, 548, 552, 557, 562, 566, 570, 574, 578, 582, 586, 590, 594, 598, 602, 606, 610, 614, 618
  - lib/widgets/center_loading.dart: 12, 14, 15, 16, 18

Expected behavior

either to remove % and work as usual 0r to throw a clear exception to tell the developer what is wrong with his workflow

@maxzod
Copy link
Author

maxzod commented Jun 16, 2023

if I am right about the Expected behavior I could open a PR to fix it

@alestiago
Copy link
Contributor

alestiago commented Jun 26, 2023

Hi @maxzod ! Thanks for opening an issue!

I don't think we should allow %. I think the input should only be numeric, not a string.

We are not planning to work on this ourselves since this is easily resolvable by using a sensible input type. If you wish to work on some error logic to ensure the input is numeric, we're willing to accept the feature.

@maxzod
Copy link
Author

maxzod commented Jun 26, 2023

@alestiago
Great, I will work on it

@alestiago alestiago added the waiting for response Waiting for follow up label Sep 4, 2023
@alestiago alestiago changed the title adding % results un expected behavior fix: catch non-numeric min_coverage input Sep 11, 2023
@alestiago alestiago added good first issue Good for newcomers and removed waiting for response Waiting for follow up labels Sep 18, 2023
@alestiago
Copy link
Contributor

@maxzod I've unassigned you for now due to inactivity, if you wish to resume the work here let me know. Anyone else is also welcome to work on this Pull Request, let us know here with a comment 💙 !

@kelvinwieth
Copy link
Contributor

Hey @alestiago, can I work on this one?

@alestiago
Copy link
Contributor

This has now been resolved by #290, the change will be visible when a new release is available.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
3 participants