-
Notifications
You must be signed in to change notification settings - Fork 428
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
Update XEP-0050: Ad-Hoc Commands #4043
Conversation
small_tests_24 / small_tests / 06c5b94 small_tests_25 / small_tests / 06c5b94 small_tests_25_arm64 / small_tests / 06c5b94 dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / 06c5b94 ldap_mnesia_24 / ldap_mnesia / 06c5b94 dynamic_domains_mysql_redis_25 / mysql_redis / 06c5b94 ldap_mnesia_25 / ldap_mnesia / 06c5b94 dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / 06c5b94 dynamic_domains_mssql_mnesia_25 / odbc_mssql_mnesia / 06c5b94 muc_light_http_api_SUITE:end_per_suite{error,
{{unregistering_failed,
{amount,1},
{unregistered_items,
[{{<<"_invite_to_room_2583">>,
[{escalus_event_mgr,<0.16846.2>},
{tc_name,invite_to_room},
{escalus_cleaner,<0.16845.2>},
{watchdog,<0.16844.2>},
{mim_data_dir,
"/home/circleci/project/big_tests/tests/muc_light_http_api_SUITE_data"},
{preset,"odbc_mssql_mnesia"},
{tc_logfile,
"https://circleci-mim-results.s3.eu-central-1.amazonaws.com/PR/4043/178491/odbc_mssql_mnesia.25.3-amd64/big/ct_run.test%408758c638b3b0.2023-06-22_08.35.40/big_tests.tests.muc_light_http_api_SUITE.logs/run.2023-06-22_08.47.56/muc_light_http_api_suite.invite_to_room.203266.html"},
{tc_group_properties,[{name,positive},parallel]},
{tc_group_path,[]},
{data_dir,
"/home/circleci/project/big_tests/_build/default/lib/mongoose_tests/ebin/muc_light_http_api_SUITE_data/"},
{priv_dir,
"https://circleci-mim-results.s3.eu-central-1.amazonaws.com/PR/4043/178491/odbc_mssql_mnesia.25.3-amd64/big/ct_run.test%408758c638b3b0.2023-06-22_08.35.40/big_tests.tests.muc_light_http_api_SUITE.logs/run.2023-06-22_08.47.56/log_private/"},
{{saved_modules,mongooseim@localhost,<<"test type">>},
#{mod_adhoc => #{iqdisc => one_queue,report_commands_node => false},
mod_amp => #{},
mod_bosh =>
#{backend => mnesia,inactivity => 30,max_pause => 120,
max_wait => infinity,server_acks => false},
mod_cache_users =>
#{number_of_segments => 5,strategy => fifo,time_to_live => 2},
mod_carboncopy => #{iqdisc => no_queue},
mod_disco =>
#{extra_domains => [],iqdisc => one_que... elasticsearch_and_cassandra_25 / elasticsearch_and_cassandra_mnesia / 06c5b94 pgsql_mnesia_24 / pgsql_mnesia / 06c5b94 internal_mnesia_25 / internal_mnesia / 06c5b94 pgsql_mnesia_25 / pgsql_mnesia / 06c5b94 mssql_mnesia_25 / odbc_mssql_mnesia / 06c5b94 mysql_redis_25 / mysql_redis / 06c5b94 |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #4043 +/- ##
==========================================
+ Coverage 83.80% 83.82% +0.02%
==========================================
Files 526 526
Lines 33129 33133 +4
==========================================
+ Hits 27764 27774 +10
+ Misses 5365 5359 -6
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is something wrong with types
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the refactor, but I think there are some issues with the rest of this PR. Apart from the specific comments, I would add that a change in functionality should be covered by tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requesting changes to avoid accidental merge.
small_tests_24 / small_tests / 15c3102 small_tests_25_arm64 / small_tests / 15c3102 small_tests_25 / small_tests / 15c3102 ldap_mnesia_24 / ldap_mnesia / 15c3102 dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / 15c3102 ldap_mnesia_25 / ldap_mnesia / 15c3102 dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / 15c3102 dynamic_domains_mysql_redis_25 / mysql_redis / 15c3102 pgsql_mnesia_24 / pgsql_mnesia / 15c3102 dynamic_domains_mssql_mnesia_25 / odbc_mssql_mnesia / 15c3102 elasticsearch_and_cassandra_25 / elasticsearch_and_cassandra_mnesia / 15c3102 internal_mnesia_25 / internal_mnesia / 15c3102 pgsql_mnesia_25 / pgsql_mnesia / 15c3102 mssql_mnesia_25 / odbc_mssql_mnesia / 15c3102 mysql_redis_25 / mysql_redis / 15c3102 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added two minor suggestions
Co-authored-by: JanuszJakubiec <54719269+JanuszJakubiec@users.noreply.github.com>
small_tests_24 / small_tests / a14fb58 small_tests_25_arm64 / small_tests / a14fb58 small_tests_25 / small_tests / a14fb58 ldap_mnesia_24 / ldap_mnesia / a14fb58 dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / a14fb58 dynamic_domains_mssql_mnesia_25 / odbc_mssql_mnesia / a14fb58 dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / a14fb58 pgsql_mnesia_24 / pgsql_mnesia / a14fb58 dynamic_domains_mysql_redis_25 / mysql_redis / a14fb58 ldap_mnesia_25 / ldap_mnesia / a14fb58 internal_mnesia_25 / internal_mnesia / a14fb58 elasticsearch_and_cassandra_25 / elasticsearch_and_cassandra_mnesia / a14fb58 mysql_redis_25 / mysql_redis / a14fb58 pgsql_mnesia_25 / pgsql_mnesia / a14fb58 mssql_mnesia_25 / odbc_mssql_mnesia / a14fb58 mam_SUITE:rdbms_async_cache_prefs_cases:run_set_and_get_prefs_cases{error,{{badmatch,0},
[{escalus_story,drop_presences,2,
[{file,"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_story.erl"},
{line,191}]},
{escalus_story,'-start_ready_clients/2-fun-0-',3,
[{file,"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_story.erl"},
{line,135}]},
{lists,foldl,3,[{file,"lists.erl"},{line,1350}]},
{escalus_story,start_ready_clients,2,
[{file,"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_story.erl"},
{line,128}]},
{lists,flatmap_1,2,[{file,"lists.erl"},{line,1335}]},
{lists,flatmap_1,2,[{file,"lists.erl"},{line,1335}]},
{escalus_story,story,4,
[{file,"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_story.erl"},
{line,69}]},
{test_server,ts_tc,3,[{file,"test_server.erl"},{line,1782}]}]}} mam_SUITE:rdbms_async_cache_prefs_cases:messages_filtered_when_prefs_default_policy_is_always{error,
{{assertion_failed,assert,is_iq_result,
[{xmlel,<<"iq">>,
[{<<"type">>,<<"set">>},
{<<"id">>,<<"2f66d914934532cb7a87dbf07110475c">>}],
[{xmlel,<<"prefs">>,
[{<<"xmlns">>,<<"urn:xmpp:mam:1">>},
{<<"default">>,<<"always">>}],
[{xmlel,<<"always">>,[],[]},{xmlel,<<"never">>,[],[]}]}]}],
{xmlel,<<"iq">>,
[{<<"from">>,
<<"alice_messages_filtered_when_prefs_default_policy_is_always_2158@localhost">>},
{<<"to">>,
<<"alice_messages_filtered_when_prefs_default_policy_is_always_2158@localhost/res1">>},
{<<"id">>,<<"pushb5b8ca52654b6090">>},
{<<"type">>,<<"set">>}],
[{xmlel,<<"query">>,
[{<<"xmlns">>,<<"jabber:iq:roster">>}],
[{xmlel,<<"item">>,
[{<<"subscription">>,<<"both">>},
{<<"jid">>,
<<"bob_messages_filtered_when_prefs_default_policy_is_always_2158@localhost">>}],
[]}]}]},
"<iq from='alice_messages_filtered_when_prefs_default_policy_is_always_2158@localhost' to='alice_messages_filtered_when_prefs_default_policy_is_always_2158@localhost/res1' id='pushb5b8ca52654b6090' type='set'><query xmlns='jabber:iq:roster'><item subscription='both' jid='bob_messages_filtered_when_prefs_default_policy_is_always_2158@localhost'/></query></iq>"},
[{escalus_new_assert,assert_true,2,
[{file,
"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_new_assert.er... mssql_mnesia_25 / odbc_mssql_mnesia / a14fb58 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍
This PR updates support for XEP-0050. The actual change that has to be made comes from Chapter 3.4 - Command Actions. More precisely these sentences:
In
adhoc
module's API we need to make sure that we won't ever return result as described.In addition to those changes I did a minor refactor so the new logic is clear.