opentelemetry-erlang
opentelemetry-erlang copied to clipboard
open-telemetry
Merge Elixir coverages
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.
I will look into it. It may be possible to just ignore that from the cover tool configuration.
One option is to add the include_apps option of covertool also to the mix coverage tool. Now it is available only for rebar.
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.
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.
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.
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 :)
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.