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

Removing message formatting rounding #611

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

Conversation

didibz-harmonya
Copy link

When defining a coverage threshold (88.45 for example) and the actual coverage is very close (88.449 for example) we will get a failure message. However, the message will be: "FAIL Required test coverage of 88.45% not reached. Total coverage: 88.45%" where it's wrong and confusing since we failed because it's under while it the message it presented as equal. The removing of the rounding will fix it.

When defining a coverage threshold (88.45 for example) and the actual coverage is very close (88.449  for example) we will get a failure message. However, the message will be: "FAIL Required test coverage of 88.45% not reached. Total coverage: 88.45%" where it's wrong and confusing since we failed because it's under while it the message it presented as equal. The removing of the rounding will fix it.
@Colin-b
Copy link

Colin-b commented Feb 1, 2024

Hello @AdiKrasin, can this fix get merged and released?

@Colin-b
Copy link

Colin-b commented Feb 7, 2024

@ionelmc Can you have a look at this? Thanks again

@Colin-b
Copy link

Colin-b commented Feb 23, 2024

If you are not ok with this change, I'll gladly come up with a PR fixing it as well matching your expectations. This is a long standing issue that is quite painful with long running test suites (because it's a pain to relaunch it to know the coverage) that are distributed (so cannot run with coverage only).

I'd argue a more in-line change would be to read the coverage "precision" setting and report it the same way coverage is. Is that something you would be willing to merge and release?

@@ -341,7 +341,7 @@ def pytest_terminal_summary(self, terminalreporter):
markup = {'red': True, 'bold': True} if failed else {'green': True}
message = (
'{fail}Required test coverage of {required}% {reached}. '
'Total coverage: {actual:.2f}%\n'
'Total coverage: {actual}%\n'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would fix the problem you described without changing the output too dramatically.

Suggested change
'Total coverage: {actual}%\n'
'Total coverage: {truncate(actual, 2)}%\n'

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right !

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could just reuse that from coverage? Hopefully @nedbat won't change it soon :-)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am actively refactoring that file to make more of it public: https://github.com/nedbat/coveragepy/tree/nedbat/public-analysis . We could talk about how to make it useful here.

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.

None yet

6 participants