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

Merge Elixir coverages #185

Open
tsloughter opened this issue Dec 7, 2020 · 7 comments · May be fixed by #610
Open

Merge Elixir coverages #185

tsloughter opened this issue Dec 7, 2020 · 7 comments · May be fixed by #610

Comments

@tsloughter
Copy link
Member

The top level Elixir tests treat both apps as deps and has an empty coverage because of that.

The cover data we get in codecov is thus incomplete which is frustrating and results in caring less about what codecov says which will be detrimental in the long run.

@hauleth
Copy link
Member

hauleth commented Dec 7, 2020

I will look into it. It may be possible to just ignore that from the cover tool configuration.

@albertored
Copy link
Contributor

One option is to add the include_apps option of covertool also to the mix coverage tool. Now it is available only for rebar.

@albertored albertored linked a pull request Aug 23, 2023 that will close this issue
@bryannaegele
Copy link
Contributor

Maybe a dumb question. Is this something we actually care about? It's 90% wrapper of erlang calls, so I'm not sure I see the benefit to adding this metric.

@albertored
Copy link
Contributor

Ideally we should cover everything :-)

If we add a new Erlang function and the corresponding Elixir wrapper we should cover it, there can be typos like swapping arguments o missing ones. It can also happen that an Erlang function changes its signature and the elixir one is no longer aligned (not so frequently, it would probably be a breaking change). I see the benefits in knowing which code paths are covered also if they are simple wrappers.

@bryannaegele
Copy link
Contributor

Codecov only covers test coverage though and we don't write tests for wrapper functions or passthroughs. Dialyzer would pick up any incorrect calls. I'll admit I'm coming from this from a personal belief that "codecov is a useless automated metric for PRs" which I'm trying to set aside haha, but I'm trying to see the benefit of running something that's going to show like 10% coverage or whatever.

@tsloughter
Copy link
Member Author

I like being able to point people to tests when they ask how to use something. Sort of like tests in docs that people do. So I'd be happy if we had tests of all those wrappers :)

@albertored
Copy link
Contributor

I'm used to work with 100% coverage and PR blocked if the coverage decreases. This implies covering also code path that seems more "trivial" but it happened more than once that in that trivial parts there was a bug or a bug appeared after a while.

In this particular case I agree with you @bryannaegele that bugs in elixir wrapper should already be detected by dialyzer but never say never :)

Anyway the linked PR is a way of adding the coverage but clearly feel free to reject it if you think is an overkill.

Note on the 100% coverage: in elixir it is easier because the de facto tool used (excoveralls) allows to exclude certain lines from the report and this is useful to skip some very trivial code or some error very difficult to be triggered in tears. It is a pity that the Erlang cover module does not have this feature.

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 a pull request may close this issue.

4 participants