erlang: migrate test runner to json from term specs

Summary:
This switches test runner from using term test specs to using JSON. This should help lower memory pressure from buck2 when running tests since constructing the test specs as terms turned out pretty expensive in other cases.

The other changes are related to the fact that now we pass-in data as binaries rather than strings.
In particular the `erl` command was concatenated into a single string on buck side, only to be re-split on the erlang side - instead we pass it in as an array of strings now.

Additionally, I switched code loading of hooks to be more assertive - before it would fail silently and then result in suspicious failures - failing early makes it much easier to understand what's going on.

Writing the test spec file was simplified to do just one IO operation, rather than a sequence of writes for key config value.

Reviewed By: TD5

Differential Revision: D59328376

fbshipit-source-id: 9c9e3373babea50741c89bbddf3315104847f0ed

erlang/common_test/common/include/buck_ct_records.hrl

@@ -8,12 +8,12 @@
 %% % @format
 
 -record(test_info, {
-    dependencies :: [string()],
-    test_suite :: string(),
-    config_files :: [string()],
+    dependencies :: [file:filename()],
+    test_suite :: binary(),
+    config_files :: [binary()],
     providers :: [{atom(), [term()]}],
     ct_opts :: [term()],
-    erl_cmd :: string(),
+    erl_cmd :: [binary()],
     extra_flags :: [string()],
     common_app_env :: #{string() => string()},
     artifact_annotation_mfa :: artifact_annotations:annotation_function()
@@ -31,7 +31,7 @@
     providers :: [{module(), [term()]}],
     ct_opts :: [term()],
     common_app_env :: #{string() => string()},
-    erl_cmd :: string(),
+    erl_cmd :: [binary()],
     extra_flags :: [string()],
     artifact_annotation_mfa :: artifact_annotations:annotation_function()
 }).

erlang/common_test/common/src/buck_ct_parser.erl

@@ -18,12 +18,12 @@
 %% Public API
 -export([parse_str/1]).
 
--spec parse_str(string()) -> term().
-parse_str("") ->
+-spec parse_str(binary()) -> term().
+parse_str(<<"">>) ->
     [];
 parse_str(StrArgs) ->
     try
-        {ok, Tokens, _} = erl_scan:string(StrArgs ++ "."),
+        {ok, Tokens, _} = erl_scan:string(unicode:characters_to_list([StrArgs, "."])),
         erl_parse:parse_term(Tokens)
     of
         {ok, Term} ->
@@ -34,7 +34,7 @@ parse_str(StrArgs) ->
         E:R:S ->
             error(
                 lists:flatten(
-                    io_lib:format("Error parsing StrArgs ~p, error ~p", [StrArgs, erl_error:format_exception(E, R, S)])
+                    io_lib:format("Error parsing StrArgs ~p, error ~ts", [StrArgs, erl_error:format_exception(E, R, S)])
                 )
             )
     end.

erlang/common_test/test_binary/src/list_test.erl

@@ -93,8 +93,7 @@ test_exported_test(Suite, Test) ->
 
 -spec load_hooks([module()]) -> ok.
 load_hooks(Hooks) ->
-    lists:map(fun code:ensure_loaded/1, Hooks),
-    ok.
+    ok = code:ensure_modules_loaded(Hooks).
 
 %% We extract the call to the groups() method so that we can type it.
 -spec suite_groups(suite(), [module()]) -> groups_output().

erlang/common_test/test_binary/src/test_binary.erl

@@ -77,28 +77,27 @@ main(Other) ->
 
 -spec load_test_info(string()) -> #test_info{}.
 load_test_info(TestInfoFile) ->
-    {ok, [
-        #{
-            "dependencies" := Dependencies,
-            "test_suite" := SuiteName,
-            "test_dir" := TestDir,
-            "config_files" := ConfigFiles,
-            "providers" := Providers,
-            "ct_opts" := CtOpts,
-            "extra_ct_hooks" := ExtraCtHooks,
-            "erl_cmd" := ErlCmd,
-            "extra_flags" := ExtraFlags,
-            "artifact_annotation_mfa" := ArtifactAnnotationMFA,
-            "common_app_env" := CommonAppEnv
-        }
-    ]} = file:consult(TestInfoFile),
+    {ok, Content} = file:read_file(TestInfoFile),
+    #{
+        <<"dependencies">> := Dependencies,
+        <<"test_suite">> := SuiteName,
+        <<"test_dir">> := TestDir,
+        <<"config_files">> := ConfigFiles,
+        <<"providers">> := Providers,
+        <<"ct_opts">> := CtOpts,
+        <<"extra_ct_hooks">> := ExtraCtHooks,
+        <<"erl_cmd">> := ErlCmd,
+        <<"extra_flags">> := ExtraFlags,
+        <<"artifact_annotation_mfa">> := ArtifactAnnotationMFA,
+        <<"common_app_env">> := CommonAppEnv
+    } = json:decode(Content),
     Providers1 = buck_ct_parser:parse_str(Providers),
     CtOpts1 = make_ct_opts(
         buck_ct_parser:parse_str(CtOpts),
         [buck_ct_parser:parse_str(CTH) || CTH <- ExtraCtHooks]
     ),
     #test_info{
-        dependencies = [filename:absname(Dep) || Dep <- Dependencies],
+        dependencies = [unicode:characters_to_list(filename:absname(Dep)) || Dep <- Dependencies],
         test_suite = filename:join(filename:absname(TestDir), [SuiteName, ".beam"]),
         config_files = lists:map(fun(ConfigFile) -> filename:absname(ConfigFile) end, ConfigFiles),
         providers = Providers1,
@@ -109,9 +108,9 @@ load_test_info(TestInfoFile) ->
         common_app_env = CommonAppEnv
     }.
 
--spec parse_mfa(string()) -> artifact_annotations:annotation_function() | {error, term()}.
+-spec parse_mfa(binary()) -> artifact_annotations:annotation_function() | {error, term()}.
 parse_mfa(MFA) ->
-    case erl_scan:string(MFA) of
+    case erl_scan:string(unicode:characters_to_list(MFA)) of
         {ok,
             [
                 {'fun', _},
@@ -144,10 +143,11 @@ parse_mfa(MFA) ->
 make_ct_opts(CtOpts, []) -> CtOpts;
 make_ct_opts(CtOpts, ExtraCtHooks) -> [{ct_hooks, ExtraCtHooks} | CtOpts].
 
--spec load_suite(string()) -> [{atom(), string()}].
+-spec load_suite(binary()) -> atom().
 load_suite(SuitePath) ->
-    {module, Module} = code:load_abs(filename:rootname(filename:absname(SuitePath))),
-    {Module, filename:absname(SuitePath)}.
+    Path = unicode:characters_to_list(filename:rootname(filename:absname(SuitePath))),
+    {module, Module} = code:load_abs(Path),
+    Module.
 
 -spec get_hooks(#test_info{}) -> [module()].
 get_hooks(TestInfo) ->
@@ -176,7 +176,7 @@ running(TestInfoFile, OutputDir, Tests) ->
 
 get_listing(TestInfo, OutputDir) ->
     code:add_paths(TestInfo#test_info.dependencies),
-    {Suite, _Path} = load_suite(TestInfo#test_info.test_suite),
+    Suite = load_suite(TestInfo#test_info.test_suite),
 
     {ok, ProjectRoot} = file:get_cwd(),
     true = os:putenv("PROJECT_ROOT", ProjectRoot),

erlang/common_test/test_binary/src/test_runner.erl

@@ -22,7 +22,7 @@
 -spec run_tests([string()], #test_info{}, string(), [#test_spec_test_case{}]) -> ok.
 run_tests(Tests, #test_info{} = TestInfo, OutputDir, Listing) ->
     check_ct_opts(TestInfo#test_info.ct_opts),
-    Suite = list_to_atom(filename:basename(TestInfo#test_info.test_suite, ".beam")),
+    Suite = binary_to_atom(filename:basename(TestInfo#test_info.test_suite, ".beam")),
     StructuredTests = lists:map(fun(Test) -> parse_test_name(Test, Suite) end, Tests),
     case StructuredTests of
         [] ->
@@ -62,10 +62,8 @@ execute_test_suite(
         Suite, Tests, filename:absname(filename:dirname(SuitePath)), OutputDir, CtOpts
     ),
     TestSpecFile = filename:join(OutputDir, "test_spec.spec"),
-    lists:foreach(
-        fun(Spec) -> file:write_file(TestSpecFile, io_lib:format("~tp.~n", [Spec]), [append]) end,
-        TestSpec
-    ),
+    FormattedSpec = [io_lib:format("~tp.~n", [Entry]) || Entry <- TestSpec],
+    file:write_file(TestSpecFile, FormattedSpec),
     NewTestEnv = TestEnv#test_env{test_spec_file = TestSpecFile, ct_opts = CtOpts},
     try run_test(NewTestEnv) of
         ok -> ok
@@ -335,7 +333,8 @@ add_or_append(List, {Key, Value}) ->
 %% @doc Built the test_spec selecting the requested tests and
 %% specifying the result output.
 -spec build_test_spec(atom(), [atom()], string(), string(), [term()]) -> [term()].
-build_test_spec(Suite, Tests, TestDir, OutputDir, CtOpts) ->
+build_test_spec(Suite, Tests, TestDir0, OutputDir, CtOpts) ->
+    TestDir = unicode:characters_to_list(TestDir0),
     ListGroupTest = get_requested_tests(Tests),
     SpecTests = lists:map(
         fun

erlang/common_test/test_exec/src/ct_daemon_node.erl

@@ -74,7 +74,7 @@ start(
     % We should forward emu flags here,
     % see T129435667
     Port = ct_runner:start_test_node(
-        os:find_executable("erl"),
+        [os:find_executable("erl")],
         [],
         CodePaths,
         ConfigFiles,

erlang/common_test/test_exec/src/ct_runner.erl

@@ -210,7 +210,7 @@ common_app_env_args(Env) ->
     lists:append([["-common", Key, Value] || {Key, Value} <- maps:to_list(Env)]).
 
 -spec start_test_node(
-    Erl :: string(),
+    Erl :: [binary()],
     ExtraFlags :: [string()],
     CodePath :: [file:filename_all()],
     ConfigFiles :: [file:filename_all()],
@@ -236,7 +236,7 @@ start_test_node(
     ).
 
 -spec start_test_node(
-    Erl :: string(),
+    Erl :: [binary()],
     ExtraFlags :: [string()],
     CodePath :: [file:filename_all()],
     ConfigFiles :: [file:filename_all()],
@@ -254,7 +254,7 @@ start_test_node(
     ReplayIo
 ) ->
     % split of args from Erl which can contain emulator flags
-    [_Executable | Flags] = string:split(ErlCmd, " ", all),
+    [_Executable | Flags] = ErlCmd,
     % we ignore the executable we got, and use the erl command from the
     % toolchain that executes this code
     ErlExecutable = os:find_executable("erl"),

erlang/erlang_tests.bzl

@@ -31,7 +31,6 @@ load(
     "file_mapping",
     "list_dedupe",
     "preserve_structure",
-    "to_term_args",
 )
 
 def erlang_tests_macro(
@@ -270,43 +269,36 @@ def _write_test_info_file(
         test_dir: Artifact,
         config_files: list[Artifact],
         erl_cmd: [cmd_args, Artifact]) -> Artifact:
+    dependency_paths = _list_code_paths(dependencies)
+    dependency_paths.extend(extra_code_paths)
     tests_info = {
         "artifact_annotation_mfa": ctx.attrs._artifact_annotation_mfa,
         "common_app_env": ctx.attrs.common_app_env,
         "config_files": config_files,
         "ct_opts": ctx.attrs._ct_opts,
-        "dependencies": _list_code_paths(extra_code_paths, dependencies),
-        "erl_cmd": cmd_args(['"', cmd_args(erl_cmd, delimiter = " "), '"'], delimiter = ""),
+        "dependencies": dependency_paths,
+        "erl_cmd": erl_cmd,
         "extra_ct_hooks": ctx.attrs.extra_ct_hooks,
         "extra_flags": ctx.attrs.extra_erl_flags,
         "providers": ctx.attrs._providers,
         "test_dir": test_dir,
         "test_suite": test_suite,
     }
     test_info_file = ctx.actions.declare_output("tests_info")
-    ctx.actions.write(
-        test_info_file,
-        to_term_args(tests_info),
-    )
+    ctx.actions.write_json(test_info_file, tests_info)
     return test_info_file
 
-def _list_code_paths(extra_code_paths: list[Artifact], dependencies: ErlAppDependencies) -> list[cmd_args]:
+def _list_code_paths(dependencies: ErlAppDependencies) -> list[[Artifact, cmd_args]]:
     """lists all ebin/ dirs from the test targets dependencies"""
     folders = []
-    for path in extra_code_paths:
-        folders.append(cmd_args(path, format = '"{}"'))
     for dependency in dependencies.values():
         if ErlangAppInfo in dependency:
             dep_info = dependency[ErlangAppInfo]
-            if dep_info.virtual:
-                continue
-            folders.append(cmd_args(
-                dep_info.app_folder,
-                format = '"{}/ebin"',
-            ))
+            if not dep_info.virtual:
+                folders.append(cmd_args(dep_info.app_folder, format = "{}/ebin", delimiter = ""))
         elif ErlangTestInfo in dependency:
             dep_info = dependency[ErlangTestInfo]
-            folders.append(cmd_args(dep_info.output_dir, format = '"{}"'))
+            folders.append(dep_info.output_dir)
     return folders
 
 def _build_resource_dir(ctx, resources: list, target_dir: str) -> Artifact: