rebar3 icon indicating copy to clipboard operation
rebar3 copied to clipboard

Do not load applications from CT provider

Open dmitrivereshchagin opened this issue 1 year ago • 3 comments

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.

dmitrivereshchagin avatar Nov 27 '22 21:11 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: https://github.com/erlang/rebar3/commit/854abc1bda6457ababae18d115e2aed26cce6405

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 avatar Nov 27 '22 22:11 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 [...]

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),

dmitrivereshchagin avatar Nov 28 '22 04:11 dmitrivereshchagin

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: https://github.com/erlang/rebar3/blob/ac6f8bcbc64a09849f4ac7867c8272b90683ff2e/apps/rebar/src/rebar_prv_eunit.erl#L524-L550

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:

https://github.com/erlang/rebar3/blob/ac6f8bcbc64a09849f4ac7867c8272b90683ff2e/apps/rebar/src/rebar_utils.erl#L467-L474

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.

ferd avatar Nov 28 '22 14:11 ferd