Do not load applications from CT provider

[dmitrivereshchagin]

Currenlty CT provider tries to load each application mentioned in sys_config files. This may silently fail (and in most cases it does) if the application is not found in the code path. At the same time a provider that we can run before CT can add the application directory into the code path and make it happen.

For exmaple, if we want to tweak configuration for one of our test suites, we can put something like the following in init_per_suite/1:

ok = application:load(acme),
ok = application:set_env(acme, param, SomeValue),
{ok, _} = application:ensure_all_started(acme),

Then rebar3 ct will pass. But rebar3 do eunit, ct will fail, since the application will already be loaded.

However, the problem is not with the code path itself. Seems like CT provider should not load applications from sys_config files. This way it will mimic -config option more closely: application is not loaded, application parameters read from file persisted in application controller state and won't be available until the application is loaded. It will also fix rebar3 do eunit, ct in the above example.

Please take a look on tests in tsloughter/rebar3_tests#18.

Marked as draft, since shell tests workflow references rebar3_tests fork.

[ferd]

The applications are specifically loaded in order to apply the configs to them, because the configs do not get applied if the apps aren't loaded: 854abc1

This is in no small part because people kept complaining about things not working and being surprised by it.

This seems to undo a feature that was put there on purpose and risks breaking tons of test suites in random OSS repositories. I'm not sure we can afford to change this without exploding a significant part of the community's test suites.

[ferd]

This is specifically something that shouldn't be undone because it ensures that if there is a path clash across dependencies and plugins (because a plugin uses a dep from a different version from the main application), the dependency gets loaded at a higher priority than the plugin for test execution.

[dmitrivereshchagin]

Not sure if it's needed here without application loading. There's the same call in do/2 just before running test suites.

[dmitrivereshchagin]

The applications are specifically loaded in order to apply the configs to them, because the configs do not get applied if the apps aren't loaded [...]

I said above that "application parameters read from file persisted in application controller state and won't be available until the application is loaded". But this is the way -config option is handled. In our case these parameters will be available even though the application is not loaded (please take a look at CT suite from tsloughter/rebar3_tests#18, I updated it).

It seems like the only thing missing when application is not loaded here is the configuration defined by env-tuple from application specification (currently for rebar3 ct from the example above it will be missing too).

Proposed change is not fully backward compatible. It can break test suits not loading applications explicitly. I think, however, that to consistently load applications for both cases of the above example is more dangerous.

Anyway if we leave it as is, the issue can be fixed in test suite itself:

_  = application:unload(acme),
ok = application:load(acme),

[ferd]

I am not necessarily willing to break people's test suites and then field all the complaints about it. We have to think hard about that because this risks breaking a lot of stuff with no clear indication of what the fix is supposed to be.

I have to be honest that I'm considering part of this being: we made a mistake in forcing this, and now we have to live with it for the foreseeable future, or until a next major release. Having people with dozens of projects have all their tests start breaking and having to search for all the places where they need to manually add unload+load calls is going to be rather tricky of a proposal for a minor point version.

OTOH we also have to make sure this is applied consistently. For example, eunit is doing the same exact thing and the behavior is currently consistent across providers support sys.config support:

rebar3/apps/rebar/src/rebar_prv_eunit.erl

Lines 524 to 550 in ac6f8bc

	apply_sys_config(State) ->
	CfgSysCfg = case rebar_state:get(State, eunit_opts, []) of
	Opts when is_list(Opts) ->
	case proplists:get_value(sys_config, Opts, []) of
	[] -> [];
	L when is_list(hd(L)) -> L;
	S when is_list(S) -> [S]
	end;
	_ ->
	[]
	end,
	{RawOpts, _} = rebar_state:command_parsed_args(State),
	SysCfgs = rebar_string:lexemes(
	proplists:get_value(sys_config, RawOpts, ""),
	[$,]
	) ++ CfgSysCfg,
	Configs = lists:flatmap(
	fun(Filename) -> rebar_file_utils:consult_config(State, Filename) end,
	SysCfgs
	),
	%% NB: load the applications (from user directories too) to support OTP < 17
	%% to our best ability.
	rebar_paths:set_paths([deps, plugins], State),
	[application:load(Application) || Config <- Configs, {Application, _} <- Config],
	rebar_utils:reread_config(Configs, [update_logger]),
	ok.

Overall, this whole ordeal was on the list of reasons why I never even wanted to support the damn sys.config stuff in test suites in the first place. It's a weird idea because the config may change from test to test. The only way out of this quagmire may be to get rid of the force loading now that we no longer need to support OTP < 17:

rebar3/apps/rebar/src/rebar_utils.erl

Lines 467 to 474 in ac6f8bc

	%% NB: we attempt to mimic -config here, which survives app reload,
	%% hence {persistent, true}.
	SetEnv = case version_tuple(?MODULE:otp_release()) of
	{X, _, _} when X < 17 ->
	fun application:set_env/3;
	_ ->
	fun (App, Key, Val) -> application:set_env(App, Key, Val, [{persistent, true}]) end
	end,

The only reason we needed to load apps first was that before OTP 17, persistent term configuration couldn't be stored, so loading and unloading an app reset its config every time and sys.config kept losing its data. So supporting sys.config stuff required not unloading the app between each suite and needed to load the app for the config to take effect.

By explicitly saying "this was OTP 17 support and isn't required now" we could attempt the breakage (if done uniformly across all providers including eunit) and then warn in the release notes about incidentally load-bearing features, but all of this sounds like a pain in the ass.