Fix an issue with the code coverage in Erlang.mk

[andrei-mihaila]

It seems that just purging away the meck generated module (in meck:unload) is not removing it from the cover data. Exporting the cover data using cover:export/1 will also export data about the meck generated module. Erlang.mk uses the export & import flow to generate the HTML coverage report, so the report will show the purged module - which is not correct.

This might be due to an issue in the coverage tool and it would be better to investigate and fix it there, but this fix shouldn't break anything, so I think it is acceptable.

[eproxus]

Can you help me understand the problem a bit more? It seems this patch deletes the coverage data of the original module, which would delete any coverage collected before that module being mocked by Meck.

Cover-compiling a module should result in that module being in the exported coverage data, even if it has been mocked in the meantime. What shouldn't be included in the coverage data is the executed lines of the mock itself during its replacement.

[andrei-mihaila]

Yes, sure. I meant to the begin with, but I was just busy with work.

The issue is that, in an app managed with Erlang.mk and that uses Meck to mock some modules, the code coverage shows modules such as <module>_meck_original. And the percentages seem off. Here is an example.

Example Erlang.mk app

# Makefile
TEST_DEPS = meck

include erlang.mk

% src/coverage_issue.erl
-module(coverage_issue).

-export([a/0, b/0]).

a() ->
    a.

b() ->
    b.

% src/coverage_issue_other.erl
-module(coverage_issue_other).

-export([c/0]).

c() ->
    c.

% test/coverage_issue_test.erl
-module(coverage_issue_test).
-include_lib("eunit/include/eunit.hrl").

coverage_issue_test() ->
    meck:expect(coverage_issue, b, 0, b_mocked),

    ?assertEqual(a, coverage_issue:a()),
    ?assertEqual(b_mocked, coverage_issue:b()),

    ?assertEqual(c, coverage_issue_other:c()),

    meck:unload(coverage_issue).

make tests COVER=1

And the link for the coverage_issue_meck_original points to a file that doesn't exist.

With the fix applied

# Makefile
TEST_DEPS = meck

dep_meck = git https://github.com/andrei-mihaila/meck master

include erlang.mk

make distclean tests COVER=1

Added distclean to remove the deps and force the replacement of meck with the one just set in the Makefile.

Tried calling a function before mocking.

-module(coverage_issue_test).
-include_lib("eunit/include/eunit.hrl").

coverage_issue_test() ->
    % added this line
    ?assertEqual(b, coverage_issue:b()),

    meck:expect(coverage_issue, b, 0, b_mocked),

    ?assertEqual(a, coverage_issue:a()),
    ?assertEqual(b_mocked, coverage_issue:b()),

    ?assertEqual(c, coverage_issue_other:c()),

    meck:unload(coverage_issue).

The call before the mock seems to be taken into account:

I can provide an archive of the app if useful.

General issue

I think the general issue can be reproduced this way:

cover:compile("module.erl"),

% mock the module with meck which creates a new module called 'module_meck_original'
meck:expect(module, a, 0, a),

% call a mocked function (might behave the same way with or without this call)
a = module:a(),

% unload the mock which does code:delete & code:purge on 'module_meck_original'
meck:unload(module),

% export the cover data - will contain 'module_meck_original' ,
% because cover doesn't know that module was removed
cover:export(Filename) 

Thanks for looking into this!

[essen]

We are seeing this with Ra:

git clone https://github.com/rabbitmq/ra
cd ra
make ct-ra_log_2 COVER=1
make cover-report
open cover/index.html

Erlang.mk doesn't do anything special here except configure CT to enable code coverage via a spec file.

Worth noting that trying to exclude the Meck modules results in the Meck modules being present in the coverdata file, but CT then has another pass at it and removes those modules when generating its own coverage log. Erlang.mk could have a similar extra pass when generating its own cover report. But perhaps the patch in this PR (or if it's not fully correct, an improved patch similar to this) is a better way to go, I can't really think of a benefit to having Meck related modules being in the code coverage data to begin with.

[eproxus]

I realize now that I misunderstood the original fix, which I thought would remove any measurement whatsoever on the renamed meck_... module. That would be bad because Meck has a feature that collects coverage on that module and reports it back for the original name.

I'll merge and release this ASAP.

[essen]

Thank you!

[eproxus]

This is now released in 1.0.0.