Concuerror icon indicating copy to clipboard operation
Concuerror copied to clipboard

Expose concuerror_loader:load_binary/3

Open hauleth opened this issue 5 years ago • 6 comments

Summary

This will allow to provide hack for implementing support for ExUnit tests which are composed of dynamically generated modules (see #299).

Checklist

  • [ ] Has tests (or doesn't need them)
  • [x] Updates CHANGELOG (or too minor)
  • [x] References related Issues

hauleth avatar Jan 11 '19 14:01 hauleth

Codecov Report

Merging #301 into master will decrease coverage by 93.07%. The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #301       +/-   ##
==========================================
- Coverage   95.74%   2.67%   -93.08%     
==========================================
  Files          12      12               
  Lines        3668    3661        -7     
==========================================
- Hits         3512      98     -3414     
- Misses        156    3563     +3407
Flag Coverage Δ
#tests ?
#tests_real ?
#unit_tests 2.67% <0%> (-0.01%) :arrow_down:
Impacted Files Coverage Δ
src/concuerror_loader.erl 0% <0%> (-93.83%) :arrow_down:
src/concuerror_window_average.erl 0% <0%> (-100%) :arrow_down:
src/concuerror_io_lib.erl 0% <0%> (-98.64%) :arrow_down:
src/concuerror_scheduler.erl 0% <0%> (-97.14%) :arrow_down:
src/concuerror_instrumenter.erl 0% <0%> (-96.1%) :arrow_down:
src/concuerror_callback.erl 0% <0%> (-95.47%) :arrow_down:
src/concuerror_estimator.erl 0% <0%> (-95.05%) :arrow_down:
src/concuerror_inspect.erl 0% <0%> (-91.9%) :arrow_down:
src/concuerror_options.erl 9.01% <0%> (-89.81%) :arrow_down:
src/concuerror_dependencies.erl 0% <0%> (-89.19%) :arrow_down:
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 053cfed...04c196d. Read the comment docs.

codecov[bot] avatar Jan 11 '19 14:01 codecov[bot]

Codecov Report

Merging #301 into master will decrease coverage by 0.05%. The diff coverage is 57.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #301      +/-   ##
==========================================
- Coverage   95.74%   95.69%   -0.06%     
==========================================
  Files          12       12              
  Lines        3668     3670       +2     
==========================================
  Hits         3512     3512              
- Misses        156      158       +2
Flag Coverage Δ
#tests 95.34% <57.14%> (-0.08%) :arrow_down:
#tests_real 84.87% <57.14%> (-0.08%) :arrow_down:
#unit_tests 2.67% <0%> (-0.01%) :arrow_down:
Impacted Files Coverage Δ
src/concuerror_loader.erl 91.56% <57.14%> (-2.27%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 053cfed...0efd16a. Read the comment docs.

codecov[bot] avatar Jan 11 '19 14:01 codecov[bot]

IMO, this PR is really bad. For starters, its diff is unnecessarily big (there is very little reason to move functions around). Second, and more important, it does not make sense on its own. It should instead be part of a PR that implements what #299 wants to do. If that is not implemented, for whatever reason, there is no point to add and expose the `load_binary/3' function as far as Concuerror is concerned.

kostis avatar Jan 14 '19 09:01 kostis

@kostis, I prefer to accept the PR sooner, rather than wait until 'everything is done'. I also disagree that it is 'really bad'.

Moving the few lines of code makes sense for grouping the exported functions a bit better.

However, I agree that we should document internally (not officially) why this function is exposed, and cover it with a test, along the lines of what it is used for so far, but maybe without using Elixir. Such a test can be created using https://github.com/parapluu/Concuerror/blob/5bf0fed/tests-real/suites/options/other-tests#L46 (-L54) as a template.

@hauleth, it would be great if you gave it a shot. Otherwise I'll try to find some time to do it before the end of this week.

aronisstav avatar Jan 14 '19 10:01 aronisstav

@kostis the reason to move functions around is that in original source code the functions with the same name were grouped together and separated from the private functions. So to keep that layout I needed to move functions a little.

About "making sense" on its own - the #299 is blocked on ERL-805 and it doesn't seem that it will be resolved any time soon, so the only possible solution right now is to "hack around" by instrumenting BEAM code directly, wich this function allows (using original example):

%% Define module in memory

Module = erl_syntax:attribute(erl_syntax:atom(module),[erl_syntax:atom(sample)]).
ModForm =  erl_syntax:revert(Module).

Export = erl_syntax:attribute(erl_syntax:atom(export),[erl_syntax:list([erl_syntax:arity_qualifier(erl_syntax:atom(test),erl_syntax:integer(0))])]).
ExportForm = erl_syntax:revert(Export).

% define the function body
Body = erl_syntax:atom(nil).

% define the first clause
Clause =  erl_syntax:clause([],[],[Body]).

% define the function
Function =  erl_syntax:function(erl_syntax:atom(test),[Clause]).
FunctionForm = erl_syntax:revert(Function).

{ok, Mod, Bin1} = compile:forms([ModForm,ExportForm, FunctionForm]).
code:load_binary(Mod, [], Bin1).

%% Now run concuerror on it

concuerror_loader:load_binary(Mod, '', Bin1).
concuerror:run([{module, sample}]).

Will work. This will allow me to create Elixir wrapper for tests without blockers. So I would say, that this provides requested solution for #299 using current BEAM tools available.

hauleth avatar Jan 15 '19 12:01 hauleth

Actually, I think that we can fix #299 by adding a {binary, Binary} option, usable from concuerror:run([...]) that uses the newly exposed load_binary/3 code.

aronisstav avatar Jan 15 '19 13:01 aronisstav