Add rebar3 as test xref

[paulo-ferraz-oliveira]

Pre-Check

• I did a quick search in current issues first
• I'm willing and interested in helping fix the issue

Environment

• The version of rebar3 I'm running is the latest release (3.14.1)

Current behaviour

rebar3 as test xref doesn't analyze folder test with extra_src_dirs set to [{"test", [{recursive, true}]}].

Expected behaviour

I'd expect rebar3 as test xref to:

• consider 'TEST' as defined (I don't know if as test defines TEST by default, already),
• take extra_src_dirs into account (for the analyzed directories),
• take xref_extra_paths into account (it's being done, as a reference for analysis).

Extra info

I've tracked the source of my expectations issues to https://github.com/erlang/rebar3/blob/master/src/rebar_prv_xref.erl#L99.

A request similar to mine (but for dialyzer) has already been implemented here: #2190.

[ferd]

This would not necessarily work well, because xref is now set to use runtime dependencies as a way to run, to mimic the environment that you'd get building a release with your app.

One weird thing is that extra_src_dirs are not expected to be bundled and part of an application in a release (this is specifically what the options is for) and all the modules in there are omitted from the {modules, ...} tuple in the .app file.

That means that if we ask of xref to start running against extra_src_dirs, we undo the work that tries to make it work closer to what a release does. I'm not sure this is the right way to go. I guess we could nevertheless make it work because by default there are no extra_src_dirs defined, but it's a bit odd to try to be moving in both directions at once. As you said, we make that happen for dialyzer, but dialyzer specifically can benefit from enriching the type analysis from extra test files, which I don't think xref would.

I'm not too sure how I feel about this.

[paulo-ferraz-oliveira]

Even if we don't specifically consider option extra_src_dirs for this, we could still have a (new) option to extend where xref fetches its files for analysis, right? (or probably even xref_extra_paths?). If I understand your reply correctly, you don't see value in running xref in tests, is that it?

[paulo-ferraz-oliveira]

Since a "normal" analysis would contain something like

    undefined_function_calls,
    locals_not_used,
    exports_not_used,
    deprecated_function_calls

I guess I understand your argument better in this sense:

1. if tests are crashing due to undefined_function_calls it's not a problem, since it's visible there and then - when running them,
2. if some locals aren't used it's not such a big deal (the same goes for exports),
3. if functions are deprecated this will be visible (if pertaining to it) in your "code"'s analysis.

Closing...

[ferd]

I mean you have the right to see value in xref running for tests of course, but yeah, I personally tend not to run it there. If you do want to run them there, there's still probably a way to get that going. My tendency though is to really worry more about what is in the source directories than the test directories in terms of quality.

Maybe I shouldn't have that attitude in the first place, but that's the one I brought to this discussion. I might be wrong. (I similarly don't run Dialyzer on tests mostly because I sometimes cause failures on purpose in tests to see error-handling behaviour, and Dialyzer then complains about my tests failing on purpose)

[paulo-ferraz-oliveira]

I mean you have the right to see value in xref running for tests of course, but yeah, I personally tend not to run it there.

Don't get me wrong. I accepted your arguments, they made sense.

My tendency though is to really worry more about what is in the source directories than the test directories in terms of quality.

Yeah, I used to have the same approach, and even though static analysis doesn't solve the major underlying issues (like "knowing how to write tests") it's another tool I can use to help me improve.

Maybe I shouldn't have that attitude in the first place, but that's the one I brought to this discussion. I might be wrong.

No, no, you have every right to counter my arguments. That's why the issues are important, I guess. Now there's history in case somebody has the same issues in the future.

(I similarly don't run Dialyzer on tests mostly because I sometimes cause failures on purpose in tests to see error-handling behaviour, and Dialyzer then complains about my tests failing on purpose)

I do it, and then silence dialyzer where I need to. I prefer to make my intentions explicit ("I know I'm provoking an exception here, but let me signal that to the next developer"). In any case, if it gets too convoluted I remove the analysis altogether.