Skip to content

Commit

Permalink
Fix error message
Browse files Browse the repository at this point in the history 
Prior to this commit if dotnet or mvnw failed to fetch test
dependencies, for example because dotnet isn't installed, the test setup
crashed in an unexpected way:
```
amqp_system_SUITE > dotnet
    {'EXIT',
        {badarg,
            [{lists,keysearch,
                 [rmq_nodes,1,
                  {skip,
                      "Failed to fetch .NET Core test project dependencies"}],
                 [{error_info,#{module => erl_stdlib_errors}}]},
             {test_server,lookup_config,2,
                 [{file,"test_server.erl"},{line,1779}]},
             {rabbit_ct_broker_helpers,get_node_configs,2,
                 [{file,"rabbit_ct_broker_helpers.erl"},{line,1411}]},
             {rabbit_ct_broker_helpers,enable_feature_flag,2,
                 [{file,"rabbit_ct_broker_helpers.erl"},{line,1999}]},
             {amqp_system_SUITE,init_per_group,2,
                 [{file,"amqp_system_SUITE.erl"},{line,77}]},
             {test_server,ts_tc,3,[{file,"test_server.erl"},{line,1794}]},
             {test_server,run_test_case_eval1,6,
                 [{file,"test_server.erl"},{line,1391}]},
             {test_server,run_test_case_eval,9,
                 [{file,"test_server.erl"},{line,1235}]}]}}
```

This commit improves the error message instead of failing with `badarg`.

This commit also decides to fail the test setup instead of skipping the
suite because we always want CI to execute this test and be notified
instead of silently skipping if the test can't be run.
  • Loading branch information
@ansd
ansd committed Oct 16, 2024
1 parent af15166 commit ab8814a
Showing 1 changed file with 4 additions and 4 deletions.
8 changes: 4 additions & 4 deletions deps/rabbit/test/amqp_system_SUITE.erl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -98,19 +98,19 @@ build_dotnet_test_project(Config) ->
rabbit_ct_helpers:set_config(
Config, {dotnet_test_project_dir, TestProjectDir});
_ ->
{skip, "Failed to fetch .NET Core test project dependencies"}
ct:fail({"'dotnet restore' failed", Ret})
end.

build_maven_test_project(Config) ->
TestProjectDir = filename:join([?config(data_dir, Config), "java-tests"]),
Ret = rabbit_ct_helpers:exec([TestProjectDir ++ "/mvnw", "test-compile"],
[{cd, TestProjectDir}]),
[{cd, TestProjectDir}]),
case Ret of
{ok, _} ->
rabbit_ct_helpers:set_config(Config,
{maven_test_project_dir, TestProjectDir});
{maven_test_project_dir, TestProjectDir});
_ ->
{skip, "Failed to build Maven test project"}
ct:fail({"'mvnw test-compile' failed", Ret})
end.

%% -------------------------------------------------------------------
Expand Down

6 comments on commit ab8814a

@dumbbell
Copy link
Member

Choose a reason for hiding this comment

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

The use of {skip, _} was correct. The error came from the fact that the return value of rabbit_ct_helpers:run_setup_steps() was not verified on line 72:

    Config2 = rabbit_ct_helpers:run_setup_steps(
                Config1,
                [GroupSetupStep] ++
                rabbit_ct_broker_helpers:setup_steps() ++
                rabbit_ct_client_helpers:setup_steps()),

The code should be something like:

    Ret = rabbit_ct_helpers:run_setup_steps(
                Config1,
                [GroupSetupStep] ++
                rabbit_ct_broker_helpers:setup_steps() ++
                rabbit_ct_client_helpers:setup_steps()),
    case Ret of
        {skip, _} ->
            Ret;
        Config2 ->
                ok = rabbit_ct_broker_helpers:enable_feature_flag(Config2, 'rabbitmq_4.0.0'),
                Config2
    end.

The testsuite will break in the same way if another step than build_dotnet_test_project/1 or build_maven_test_project/1 fails.

There are dozens of misuses like this one everywhere in the testsuites, that’s not the only one.

Do you want me to fix this case?

@ansd
Copy link
Member Author

@ansd ansd commented on ab8814a Oct 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

By returning {skip, _}, CI will show the job as green/successful, correct?

I do want the CI job to fail if for example dotnet couldn't be installed properly:

This commit also decides to fail the test setup instead of skipping the
suite because we always want CI to execute this test and be notified
instead of silently skipping if the test can't be run.

@dumbbell
Copy link
Member

Choose a reason for hiding this comment

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

By returning {skip, _}, CI will show the job as green/successful, correct?

Probably. Locally, skipped tests do not cause ct_run to exit with non-zero. I wonder if it has an option to consider skipped tests as errors.

@ansd
Copy link
Member Author

@ansd ansd commented on ab8814a Oct 16, 2024

Choose a reason for hiding this comment

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

It seems returning {fail, Reason} is what we want. It works nicely.
Unfortunately, this return value is undocumented in https://www.erlang.org/doc/apps/common_test/ct_suite.html#c:init_per_group/2

@ansd
Copy link
Member Author

@ansd ansd commented on ab8814a Oct 16, 2024

Choose a reason for hiding this comment

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

I think it makes also sense to return {fail, Reason} instead of {skipped, Reason} in most places in rabbit_ct_broker_helpers.

@ansd
Copy link
Member Author

@ansd ansd commented on ab8814a Oct 17, 2024

Choose a reason for hiding this comment

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

I asked for clarification in erlang/otp#8952

Please sign in to comment.