-
Notifications
You must be signed in to change notification settings - Fork 517
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
base: main
Are you sure you want to change the base?
Do not load applications from CT provider #2761
Conversation
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.
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), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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 It seems like the only thing missing when application is not loaded here is the configuration defined by 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:
|
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
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
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. |
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
:Then
rebar3 ct
will pass. Butrebar3 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 fixrebar3 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.