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

Return values of init_per_suite and init_per_group #8952

Open
ansd opened this issue Oct 17, 2024 · 2 comments
Open

Return values of init_per_suite and init_per_group #8952

ansd opened this issue Oct 17, 2024 · 2 comments
Labels
enhancement team:PS Assigned to OTP team PS

Comments

@ansd
Copy link
Contributor

ansd commented Oct 17, 2024

Hello,

https://www.erlang.org/doc/apps/common_test/ct_suite.html#c:init_per_testcase/2 documents that {fail, Reason} is a valid return value.

https://www.erlang.org/doc/apps/common_test/ct_suite.html#c:init_per_suite/1 and https://www.erlang.org/doc/apps/common_test/ct_suite.html#c:init_per_group/2 do not document this return value.

However, I tested that both init_per_suite and init_per_group seem to allow {fail, Reason} and behave as desired in the sense that test cases are skipped and the process returns a non-zero exit code.

Is there a specific reason that {fail, Reason} is undocumented (and therefore {fail, Reason} shouldn't be returned from init_per_suite and init_per_group), or did you just forget to document this return value?

ansd referenced this issue in rabbitmq/rabbitmq-server Oct 17, 2024
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.
@IngelaAndin IngelaAndin added the team:PS Assigned to OTP team PS label Oct 18, 2024
@essen
Copy link
Contributor

essen commented Nov 6, 2024

I think it works because CT hooks allow the fail tuple in pre/post_init_per_suite/group. CT tries to call hooks before/after init_per_suite, and post_init_per_suite receives the result of init_per_suite. It can then modify it or fail. But the default is to just use the return value of init_per_suite as the result of post_init_per_suite, so if init_per_suite returned a fail tuple then it'll be handled as if it came from a CT hook.

It probably wasn't intentional, but since it already works and is generally useful I would say it is worth documenting and adding typespecs.

@u3s
Copy link
Contributor

u3s commented Nov 12, 2024

thanks for information, we will plan looking into this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement team:PS Assigned to OTP team PS
Projects
None yet
Development

No branches or pull requests

4 participants