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

Minimum requirement #73

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

InEdited
Copy link

@InEdited InEdited commented Apr 8, 2019

Added an option to put a minimum coverage constraint.
This resolves issue #68

  • By default minimum coverage constraint is zero
  • Number in config file represents the required minimum percentage of total coverage
  • Process exits by an error but still outputs the test report

@InEdited
Copy link
Author

InEdited commented Apr 8, 2019

Just noticed that using this feature with the one implemented in either pull requests #72 or #71 will cause the reporting to always fail, will work on that to make it work only on the tested files.

@hishamhm
Copy link
Member

hishamhm commented May 8, 2019

Just noticed that using this feature with the one implemented in either pull requests #72 or #71 will cause the reporting to always fail, will work on that to make it work only on the tested files.

#71 is now merged, could you adapt this PR to be based on the current master code instead?

Also, I think users would expect the minimum requirement % calculation to include the untested files when includeuntestedfiles is set.

@allbarbos
Copy link

Can I open another PR with the change?

@esatterwhite
Copy link

Is this still in progress. Would love to get this feature landed

@hishamhm
Copy link
Member

@allbarbos Yes, please!

(I'm very low on bandwidth for maintaining LuaCov — in fact, the previous maintainer passed away and I'm carrying the torch, but I'd love to get some maintainership help on this! If any of you wants to help out, please update this PR and let me know and I'll grant commit access)

@Tieske Tieske force-pushed the minimum-requirement branch from e77feff to 1b9a85a Compare September 7, 2023 14:44
@Tieske
Copy link
Member

Tieske commented Sep 7, 2023

@hishamhm I fixed up the PR. But it has issues beyond the simple feature;

  • the message and error value is generated in the default-reporter. Using another reporter will not behave the same, depending on how it was created.
  • moving it into the generic machinery is tricky, since you do not want an early exit because of this if the report has to be uploaded a coverage tool (eg. coveralls)

A simple option would be to leave this as it is today, and have a CI user run 2 reports. First the one they want for display/reporting. And then the default one, which will display the error.

(though that means one cannot configure the stats files to be cleared, since the second run would need those as well)

@Tieske
Copy link
Member

Tieske commented Sep 7, 2023

I'm leaning towards a separate reporter that doesn't really report anything, but just generates the error message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants