Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not load applications from CT provider #2761

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dmitrivereshchagin
Copy link
Contributor

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.

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.
@ferd
Copy link
Collaborator

ferd commented Nov 27, 2022

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.

@@ -307,10 +307,6 @@ select_tests(State, ProjectApps, CmdOpts, CfgOpts) ->
Configs = lists:flatmap(fun(Filename) ->
rebar_file_utils:consult_config(State, Filename)
end, SysConfigs),
%% NB: load the applications (from user directories too) to support OTP < 17
%% to our best ability.
rebar_paths:set_paths([deps, plugins], State),
Copy link
Collaborator

@ferd ferd Nov 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@dmitrivereshchagin
Copy link
Contributor Author

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
Copy link
Collaborator

ferd commented Nov 28, 2022

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:

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:

%% 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants