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 a CLI flag to silence warnings from :cover module #239

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

devonestes
Copy link

This introduces a new CLI flag --no-warn-cover, which will silence the warnings that we get from :cover.
Those warnings are often the result of using mocks (especially if you're using :meck), and therefore
can be ignored.

Unfortunately there's no flag or anything that turns this off in the default :cover module, which is why
we need to use our own copy of that module here. Luckily that module has barely changed in over 5
years, and so it's stable and safe to copy in this fashion.

Resolves #230

@parroty
Copy link
Owner

parroty commented Dec 13, 2020

Thanks for the PR. The cover.erl is coming from the erlang modules? Thank you for the description about compatibility, but I have some concerns that whether if this change has some implications regarding license term (and wondering if it can be applied as optional component).

I'm searching around,

@devonestes
Copy link
Author

Yep, that code comes from Erlang. I'm no lawyer, but if there's some reason you can't include that code here then I totally understand. That is unfortunately the only way to solve this issue, though, so if this doesn't work then this issue will need to be closed and marked as something that cannot be solved.

@ricardoccpaiva
Copy link

I'm facing this annoying warning but I still can't figure out the real reason of this message.

@deepfryed
Copy link

@parroty just needs attribution as both MIT and Apache 2.0 are permissive - as long as individual source files include the license and attribution. I'd like to see this get merged.

@mtwilliams
Copy link

mtwilliams commented Feb 11, 2022

We've been running into this as well. It's terribly annoying.

It looks like lib/tools/src/cover.erl has received some fixes since this was opened. Some in pernicious edge cases.

I don't like the idea of vendoring a modified version. We could upstream changes that introduce :cover.start/1, which would allow us to silence logs (silence: true).

@deepfryed
Copy link

@parroty any chance we can merge this PR soon or if you disagree close this as "won't fix" ?

@parroty
Copy link
Owner

parroty commented Jun 15, 2022

Thank you for the follow-up. I didn't have good timing to dig this up, and sorry about the lack of responses.

just needs attribution as both MIT and Apache 2.0 are permissive - as long as individual source files include the license and attribution. I'd like to see this get merged.

I am not confident about this licensing part enough yet. Is it possible for you to elaborate this "needs attribution" part? (What needs to be included in this repository?). Thanks in advance.

@deepfryed
Copy link

https://www.erlang.org/EPLICENSE

3.5. Required Notices.
You must duplicate the notice in Exhibit A in each file of the Source
Code, and this License in any documentation for the Source Code, where
You describe recipients' rights relating to Covered Code. If You
created one or more Modification(s), You may add your name as a
Contributor to the notice described in Exhibit A. 

It's my understanding that prefixing the modified cover.erl with the EPLICENSE text as a comment would be sufficient.

I don't like the idea of vendoring a modified version. We could upstream changes that introduce :cover.start/1, which would allow us to silence logs (silence: true).

I'd like that, much better option if we can get a patch upstream.

@deepfryed
Copy link

FWIW, I've got around the issue by patching cover.erl to remove the warnings and then added that under src/ in my project.

@jfacoustic
Copy link

Bump

@msaurabhee
Copy link

This needs to be merged ASAP.

@parroty
Copy link
Owner

parroty commented Jul 11, 2022

@deepfryed Thank you for the comment.

It's my understanding that prefixing the modified cover.erl with the EPLICENSE text as a comment would be sufficient.

The file in this PR (quiet_cover.erl) maintains the license section (comment lines), so just merging this PR satisfies the requirement? (Does this statement match with your understanding?)

I don't like the idea of vendoring a modified version. We could upstream changes that introduce :cover.start/1, which would allow us to silence logs (silence: true).
I'd like that, much better option if we can get a patch upstream.

I am not in favor of vendoring a modified version..., but if it can avoid license issues, I am inclined to include this feature with experimental or tentative types of notes. hmm.

@parroty
Copy link
Owner

parroty commented Jul 11, 2022

FWIW, I've got around the issue by patching cover.erl to remove the warnings and then added that under src/ in my project.

Also, this can be a reasonable workaround?

@jfacoustic
Copy link

jfacoustic commented Jul 11, 2022

@parroty I don't think that people should have to patch cover.erl to remove these warnings. The PR is here, why not just merge it? I get this all the time when using Mock, which is a fundamental part of testing to begin with.

As an alternative, we could add how to manually patch cover.erl in README.md, but IMHO it's a feature that should be a part of the official package. It's reasonable to be able to silence these warnings, especially because unnecessary console output drastically slows down test suites.

@mezz
Copy link

mezz commented Jul 11, 2022

Wouldn't the right approach here be to first upstream some changes to :cover?
https://github.com/erlang/otp/pulls

It doesn't seem right to copy all of that here.

@parroty
Copy link
Owner

parroty commented Jul 12, 2022

Wouldn't the right approach here be to first upstream some changes to :cover?
https://github.com/erlang/otp/pulls

Could anyone give a hand for this part? 🙇 Merging of this PR itself might be one solution, but it would be only for short-term (temporary) one.

As an alternative, we could add how to manually patch cover.erl in README.md, but IMHO it's a feature that should be a part of the official package. It's reasonable to be able to silence these warnings, especially because unnecessary console output drastically slows down test suites.

Also, it would be great if anyone can PR this README option, assuming that there's no harm writing the procedure as temporary solution 🙇 .

@deepfryed
Copy link

FWIW, I think cover.erl patch belongs in OTP. I'd be fine with someone submitting that patch upstream and instructions in README in the interim that can guide people on adding a patched cover.erl to src/ in their projects.

@deepfryed
Copy link

Wouldn't the right approach here be to first upstream some changes to :cover?
https://github.com/erlang/otp/pulls

Yes

it would be great if anyone can PR this README option, assuming that there's no harm writing the procedure as temporary solution

I'll send you a PR tomorrow to review, unless someone else beats me to it.

@deepfryed
Copy link

@parroty #284

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.

Getting strange warning when running mix coveralls
8 participants