Rz passthrough cover

[rzezeski]

EDIT: Collecting cover data on a mocked module is the default now. To disable the feature use no_passthrough_cover option.

This patch adds a pass_cover option which will allow coverage analysis to be collected for passthrough calls.

Feel free to nitpick the PR for anything. No ego here. Just want to see this ability in meck.

Technically, I don't think the last commit 6a7ad20a is needed, but I thought I'd leave it in and let you decide. I spent a lot of time pulling my hair trying to understand how the code server, rebar, cover, and meck all interact with each other. I'm pretty happy with this solution. I'd like it even better if I didn't have to manually tease apart the cover data but I didn't see another way.

[eproxus]

Final note:

I'm really interested in which compiler options causes the re-compilation of the abstract code to fail? I had a similar case earlier when developing meck but can't remember exactly which options caused the trouble. I'm talking to some people on the OTP team discussing possible solutions (some of which might render this whole beam dance unnecessary, hopefully).

[rzezeski]

In regards to your final note I'm going to assume you are asking about commit 6a7ad20a and my comments for cover_opts before I removed it?

The problem is that rebar, when cover is enabled, cover compiles modules with compile_beam. After that call has been make if you call Mod:module_info/0 you'll see compile options is []. So, before I even made this PR, if you had cover enabled in rebar you are compiling <name>_meck_original with NO compile opts. You would think this causes problems with defines/includes/etc but it doesn't for whatever reason. However, now that I wrote pass_cover it can't cover compile <name>_meck_original if it doesn't have debug symbols, which it won't because it was compiled with NO compile opts. This is why originally I just check for the right conditions and add debug_info but then changed that to purge the module to get to the original options used when compiling <name> (and why that works I'm not sure, the behavior of the code_server confuses me).

Do you follow? This was one of the hardest parts for me to figure out so perhaps I got something wrong and there is an easier way.

[eproxus]

Okay, let's try to structure this:

1. Original module <name> exists as a cover compile module with no options but with abstract code.
2. It is renamed to <name>_meck_original by using the abstract code.
3. We want to cover compile <name>_meck_original so that we can measure coverage:
   1. The abstract code is lost when <name>_meck_original was created since it didin't have debug_info as a compile option from cover.
   2. So we check the original module for compilation options to use.

And, when unloading:

1. We restore original cover data merged with the data collected from passthrough.

Did I get this right? :-)

[eproxus]

To summarize the paths taken by meck. In the normal case (no passthrough cover):

Loading

cover beam → get abstract → mock + original beam
Unloading
unload mock + original beam → restore cover data → re-compile cover beam

In the passthrough cover case:

Loading

cover beam → get abstract → _save beam_ →_add [debug_info]_ →mock + original beam (cover)
Unloading
unload mock + original beam → restore cover data → merge passthrough coverage → re-compile cover beam

As far as I can understand, the _save beam_ step is only to please rebar?

[eproxus]

Some thoughts:

• @bjorng at the OTP team had the idea to search the Erlang code path manually for the original beam file (with abstract code and compile options) whenever we need compiler options (or the original abstract code). Doing that could probably save tons of steps taken to get around cover's lack of compile info.
• Wouldn't it make sense to always run cover on the <name>_meck_original module? I mean, that's production code being run during testing (albeit with a different module name). The option we want to add could then be no_passthrough_cover or similar.
• And finally, for my own mental note, should all instances of the original module name inside the abstract code be renamed as well? This could prove quite tricky; there's ?MODULE or even worse things like list_to_atom("<name>"). How to detect them all?

[rzezeski]

You understand the sequence correct in your diagrams above.

The save beam step has nothing to do with pleasing rebar. Go ahead and comment out the line in meck_mod:load_binary that saves the beam and comment out the first two lines in meck:cover_original and you'll see the tests fail with something like below:

Error in process <0.86.0> with exit value: {{case_clause,{error,beam_lib,{file_error,".beam",enoent}}},[{cover,do_compile_beam,3},{cover,main_process_loop,1}]}

If you look at cover.erl you'll notice this is because cover expects to work with files. First it calls code:which which returns [] and is passed down to do_compile_beam to get_abstract_code which eventually calls beam_lib:chunks which the result of code:which which was [] and thus the enoent.

I think for this to work more smoothly someone needs to patch cover to work with binaries. In the mean time I'm not sure how else to make this work.

As for your last 3 points

1. Would it be the same as bjorng's idea if meck_mod:beam_file used code:which instead of code:get_object_code? I'm not sure but I notice your comment about code:which not working with cover compile files. I'm curious to know more about that comment.
2. I think if cover was being run on the original module then it probably makes sense to default to run cover on the meck created one. I only made it an additional option in case you wanted to keep strict backward compatibility. Thinking about it now that seems silly. However, I do like the idea of having an option to disable it in case it gives someone problems.
3. I would ask if not doing this is hurting you right now? If it's not hurting anything then I suppose leave it as is.

[rzezeski]

Is there some way to call a function even if it's not exported? If so then we could call compile_beam/2 directly and pass in a binary and things will work (downstream get_beam_file and do_compile_beam should do the right thing). If that's not possible then could we just change cover's abstract code to export that function so meck can call it? I have to retire for the night but I could try this tomorrow.

[eproxus]

1. The idea was to bypass the code module intirely; thus search code:get_path() manually. The reason that code:which/1 doesn't work for cover compiled module is because the file attribute in the module gets set to the atom cover_compiled so the original beam file cannot be found.
2. Yeah, default to cover makes sense.
3. No, not really. :-) I'm not even sure it makes sense. For example if you use the ?MODULE macro to create an ETS table, should that now be named ?MODULE_meck_original? Probably not...

No, it's not possible to call unexported functions. Messing with cover and it's abstract code I think is the last option in case everything else fails.

[rzezeski]

So with the last commit I was able to avoid saving the beam to disk. However, I had to keep it in state. No matter how hard I tried I can't find a way to load the binary from the code server without saving it to disk. Even calling code:load_binary with an actual filename (that doesn't actually exist) wasn't able to trick it into working. I looked through code_server and erl_prim_loader for something I could use but came up empty. It's weird too because reading the docs for code:get_object_code lead me to believe this is possible. If you know how to do this I'd love to know!

Anyways, even with my change things still feel a little sloppy but I wanted to share what I had thus far. I still have some other changes I want to make to try to reduce some of the code needed. I also still need to make the change to default to pass_cover and add option to turn it off.

[eproxus]

Yeah, storing it in state makes more sense.

I have plans to create a branch where I'll research the different ways of dealing with compiler options and module binaries. Right now I'm re-vamping the meck homepage a bit so that's why I don't have as much time for this as I want at the moment.

[rzezeski]

I still want to make the adjustment around the original compile options (and remove the purge and load_file calls) but I think the last 3 commits get this PR a lot closer to merge worthy. What do you think?

[eproxus]

Ah, good stuff. I'll get back to you with a review as soon as I can. :-)

[rzezeski]

FWIW, this worked for me in real tests in Riak code :)

[rzezeski]

Also, I'm happy to rebase this into one commit. I initially planned to do that but just forgot.

[eproxus]

Do you know the build / test status between commits? If they all build and pass all tests, I think it's better with smaller commits with sensible messages. :-)

[rzezeski]

No, these commits were me feeling my way through it. I see this as one logical change. All the commits were to achieve one goal. If I were to rebase them into smaller, but multiple commits it would feel arbitrary to me.

[eproxus]

So are they fine as they are or should we merge them into one big commit?

[rzezeski]

Did an interactive rebase on latest master, squashed to one commit, updated the comment a bit, and did a force push. Hope you like it :)

[eproxus]

I have to admit that I was initially put off by the sheer size of the commit, but since the majority of the code deals with hacking cover to do what we need, I think it's okay. I'd prefer if that code (which is fairly generic), is in a new module meck_cover instead though. The meck module is getting quite long these days. :-)

[eproxus]

And great work with all the patches btw. Will definately pull this in when it's ready! :-)

[rzezeski]

I think I addressed most of your concerns. Just a few comments for you to read for things I haven't changed. Once I get your blessing I'm going to squash these new commits back into the first one and then you can merge :)

[eproxus]

Looks good so far! Let's squash it and I'll take a final look at that.

[rzezeski]

How about now?

[rzezeski]

This time I'm not going to bother squashing until you say the code looks good :)

[eproxus]

Ok, looks good. I'll pull it once it's squashed. :-)

[rzezeski]

Done

[eproxus]

Awesome work! Thanks a lot!