ct: Remove usage of deprecated ct_slave in tests #9560
Conversation
The only change here I'm a bit unsure about is the `cth_auto_clean` suite, it is unclear to me why the application master is not started in `init_per_suite` already, as I can't find any call to start an app inbetween.
Since peer is way less talkative on the console, trim the remaining logs too, such that we can use e.g. `cte_track` for clean console output.
CT Test Results 2 files 57 suites 1h 11m 33s ⏱️ Results for commit 21c6109. ♻️ This comment has been updated with latest results. To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass. See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally. Artifacts// Erlang/OTP Github Action Bot |
|
I tried configuring cte_track for common_test once or twice, never finished. thanks for making an attempt! |
| @@ -1,2 +1,3 @@ | |||
| {suites,"../common_test_test",all}. | |||
| {skip_suites,"../common_test_test",[ct_release_test_SUITE],"Versions not always correct"}. | |||
| {event_handler, {cte_track, []}}. | |||
There was a problem hiding this comment.
{enable_builtin_hooks, false}. addition is not needed? no logger events messing the output?
maybe they're generated less frequently for common_test suites ...
There was a problem hiding this comment.
Hmm, I made a local test run with -enable_builtin_hooks false but it didn't look like it make much of a difference. Some logs (e.g. ct:pal tests) run on the main node and thus always end up on the terminal. I've added it regardless in case I missed something^^
There was a problem hiding this comment.
OK. maybe it is not relevant for ct.
There was a problem hiding this comment.
Do you want me to remove the addition again?
There was a problem hiding this comment.
If we're adding this enable_builtin_hoost, shouldn't we also add it to common_test.spec as well?
do you mean the .. sequence of numbers? I saw that too, maybe we could move that into a |
I mean all leftovers, after changing pal->log and taking logger events out. |
I think those are mostly gone with peer, yeah: https://github.com/erlang/otp/actions/runs/14226758185/job/39868469457 I can check this out with a closer look next week, but I think the proper fix for that is to just run the tests that test |
|
Hi @u3s, I pushed a new commit that hushes most of the remaining tests. |
| %-include_lib("common_test/include/ct.hrl"). | ||
| %-include_lib("common_test/include/ct_event.hrl"). |
There was a problem hiding this comment.
Done in the amended last commit
| end_per_testcase(_TestCase, _Config) -> | ||
| ok. | ||
| end_per_testcase(TestCase, Config) -> | ||
| ct_test_support:init_per_testcase(TestCase, Config). |
There was a problem hiding this comment.
why end_per_testcase is not called ?
There was a problem hiding this comment.
Copypaste got me :-) This is fixed now.
jchristgit
force-pushed
the
ct-peerify-tests
branch
from
a832d0a to
4f33f78
Compare
jchristgit
force-pushed
the
ct-peerify-tests
branch
from
4f33f78 to
21c6109
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR removes usage of the deprecated ct_slave and slave modules in the common_test test suite, replacing them with the peer module. It also switches from ct:pal to ct:log in most test output to better handle peer node logging and enables the cte_track event handler in test specifications.
Key changes:
- Replace
slave:start()andct_slave:start()calls withpeer:start() - Update test code to handle peer controller processes for node management
- Switch from
ct:paltoct:logfor test output logging
Reviewed Changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test_server_test_lib.erl | Updates slave node startup to use peer module with controller process handling |
| ct_test_support.erl | Major refactoring to replace slave module with peer for CT node management |
| erl2html2_SUITE.erl | Switches to ct:log and adapts to use remote node for module info calls |
| ct_verbosity_SUITE.erl | Updates node management to use peer controllers |
| Multiple test suites | Switch from ct:pal to ct:log for consistent logging behavior |
| ct_cover_SUITE_data/cover_SUITE.erl | Updates slave node management to use peer with proper cover handling |
Comments suppressed due to low confidence (1)
lib/common_test/test/ct_cover_SUITE_data/cover_SUITE.erl:176
- [nitpick] The variable name 'StartPeer' is confusing as it contains the return value of peer:start, not a function reference. Consider renaming to 'PeerResult' or 'StartResult'.
{ok, _Controller, Node} = StartPeer = erpc:call(FromNode, peer, start, [PeerOpts]),
| @@ -160,10 +165,19 @@ start_slave(Name) -> | |||
| start_slave(node(),Name). | |||
|
|
|||
| start_slave(FromNode,Name) -> | |||
There was a problem hiding this comment.
Could we rename those functions to also not include the word 'slave' while we're at it?
| slave(cleanup,_Config) -> | ||
| kill_slaves([nodename(slave,1)]). |
There was a problem hiding this comment.
Could we rename those functions to also not include the word 'slave' while we're at it?
| case test_server:is_cover() of | ||
| true -> {ok, [Node]} = cover:start(Node); | ||
| false -> ok | ||
| end, |
There was a problem hiding this comment.
ct_slave did the start like this:
MainCoverNode = cover:get_main_node(),
rpc:call(MainCoverNode,cover,start,[ENode]);
Don't we need this as well? I'm not familiar with this, so I may be wrong here.
There was a problem hiding this comment.
peer has this built-in: https://github.com/erlang/otp/blob/master/lib/stdlib/src/peer.erl#L640
| start_peer(Name, Args) -> | ||
| {ok, Host} = inet:gethostname(), | ||
| ?CT_PEER(#{host => Host, name => Name, | ||
| args => Args, timeout => timer:seconds(10)}). |
There was a problem hiding this comment.
Where is the option timeout coming from to ?CT_PEER? I only see wait_boot here: https://www.erlang.org/docs/28/apps/stdlib/peer.html#t:start_options/0
There was a problem hiding this comment.
I bet it was meant to be wait_boot (which, in the hindsight, I should've named timeout, but it's too late for that now).
| host => Host, | ||
| name => Name, | ||
| % extending some timers for slow test hosts | ||
| timeout => timer:seconds(15), |
There was a problem hiding this comment.
Where is the option timeout coming from to ?CT_PEER? I only see wait_boot here: https://www.erlang.org/docs/28/apps/stdlib/peer.html#t:start_options/0
| {init_timeout,15}, | ||
| {startup_timeout,15}]), | ||
|
|
||
| {ok, Controller, Node} = ?CT_PEER(#{name => ct_nomerge, timeout => timer:seconds(15)}), |
There was a problem hiding this comment.
Where is the option timeout coming from to ?CT_PEER? I only see wait_boot here: https://www.erlang.org/docs/28/apps/stdlib/peer.html#t:start_options/0
| {init_timeout,15}, | ||
| {startup_timeout,15}]), | ||
|
|
||
| {ok, Controller, Node} = ?CT_PEER(#{name => ct_nomerge, timeout => timer:seconds(15)}), |
There was a problem hiding this comment.
Where is the option timeout coming from to ?CT_PEER? I only see wait_boot here: https://www.erlang.org/docs/28/apps/stdlib/peer.html#t:start_options/0
| {ok,Node} = ct_slave:start(nc_remote_crash,[{boot_timeout,15}, | ||
| {init_timeout,15}, | ||
| {startup_timeout,15}]), | ||
| {ok, _Controller, Node} = ?CT_PEER(#{name => nc_remote_crash, timeout => timer:seconds(15)}), |
There was a problem hiding this comment.
Where is the option timeout coming from to ?CT_PEER? I only see wait_boot here: https://www.erlang.org/docs/28/apps/stdlib/peer.html#t:start_options/0
| @@ -37,7 +37,7 @@ | |||
| verify_events/3, verify_events/4, reformat/2, log_events/4, | |||
| join_abs_dirs/2]). | |||
|
|
|||
| -export([start_slave/3, slave_stop/1]). | |||
| -export([start_slave/3, slave_stop/2]). | |||
There was a problem hiding this comment.
Could we rename functions in this module to not use 'slave'?
These commits update the
common_testtest suite to:ct_slaveandslavemodules in tests (with the exception of thect_slavetests itself, and sometest_servertests fortest_server:start_node)ct:paltoct:login most top-level test suites (except forct_log_SUITE, which explicitly wants to test them), since the stdout ofpeernodes won't be shown on the main test nodes & IMO make sense to contain to the HTML logscte_trackevent handler in tests for both local and GitHub-based runsThe second and third commit are of course a bit opinionated, so I'm happy to drop them (and possibly make
peerlog to our stdout) if wanted.