Partial RPC cleanup in big tests #1846

erszcz · 2018-05-11T10:56:25Z

This PR tests Escalus with changes from esl/escalus#170 as well as somewhat cleans up the way RPC is done in big tests.

With regard to RPC in big tests, this is not complete. There are still things which require refactoring:

other escalus_ejabberd calls which hide RPC in the library, but by default access a particular node
rpc_apply in mam_SUITE
modules importing escalus_ejabberd, but not calling escalus_ejabberd:rpc directly - these cases weren't caught by sed
quite likely other cases, grep rpc or rg rpc in big_tests/ should give hints.

These bullet points do not suggest this stuff should go into this PR. Rather, than even when this PR is merged, the cleanup won't be complete yet.

This is a fixup for f8a01ae, which extracted some stuff to `s2s_helper`.

Commits refactoring the rest of the suites will follow.

…SUITE

codecov · 2018-05-11T13:12:41Z

Codecov Report

Merging #1846 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1846      +/-   ##
==========================================
- Coverage   74.59%   74.58%   -0.02%     
==========================================
  Files         298      298              
  Lines       27957    27957              
==========================================
- Hits        20855    20851       -4     
- Misses       7102     7106       +4

Impacted Files	Coverage Δ
..._distrib/mod_global_distrib_outgoing_conns_sup.erl	`73.91% <0%> (-8.7%)`	⬇️
...rc/global_distrib/mod_global_distrib_transport.erl	`47.05% <0%> (-5.89%)`	⬇️
src/mod_push_service_mongoosepush.erl	`82.85% <0%> (-5.72%)`	⬇️
...bal_distrib/mod_global_distrib_hosts_refresher.erl	`82.6% <0%> (-2.18%)`	⬇️
src/mod_bosh.erl	`93.27% <0%> (-1.69%)`	⬇️
src/mod_roster.erl	`79.51% <0%> (-0.25%)`	⬇️
src/mod_mam_odbc_arch.erl	`89.91% <0%> (ø)`	⬆️
src/ejabberd_c2s.erl	`84.66% <0%> (+0.21%)`	⬆️
src/mod_muc_log.erl	`77.94% <0%> (+0.25%)`	⬆️
src/mod_mam_utils.erl	`87.78% <0%> (+0.33%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0c5ffa0...3e3be2e. Read the comment docs.

michalwski

I assume that the main (and only) way of using rpc from tests is distributed_helper:rpc/4, right?
There are still some places where you changed from old escalus_ct:rpc_call to escalus_rpc:call, I guess this was on purpose.

michalwski · 2018-05-14T07:23:11Z

big_tests/tests/acc_e2e_SUITE.erl

@@ -46,16 +50,15 @@ message_test_cases() ->
    ].

 suite() ->
-    escalus:suite().
+    require_rpc_nodes([mim]) ++ escalus:suite().


That's good idea!

michalwski · 2018-05-14T07:36:26Z

big_tests/tests/distributed_helper.erl

+%%      RPCResult = rpc(mim(), remote_mod, remote_fun, [arg1, arg2]),
+%%      ...
+%%
+require_rpc_nodes(Nodes) ->


I wonder if it makes sens to ping these required nodes and fail the SUITE if any of them is not running. What do you think?

👍 for Michal's comment.

I'm against hiding IO in a function which is now completely declarative. We will know if a node is down anyway, because the RPC calls now throw instead of just returning an error by value and we will know with better granularity (the specific file, test and line).

michalwski · 2018-05-14T09:55:31Z

big_tests/tests/dynamic_modules.erl


 stop(Domain, Mod) ->
    Node = escalus_ct:get_config(ejabberd_node),
    stop(Node, Domain, Mod).

 stop(Node, Domain, Mod) ->
    Cookie = escalus_ct:get_config(ejabberd_cookie),
-    IsLoaded = escalus_ct:rpc_call(Node, gen_mod, is_loaded, [Domain, Mod], 5000, Cookie),
+    IsLoaded = escalus_rpc:call(Node, gen_mod, is_loaded, [Domain, Mod], 5000, Cookie),


Looks like 2 ways of using rpc is mixed in this module. The one imported from distributed_helper and direct use of escalus_rpc:call. Are you going to unify it in this PR or is there a reason to do so?

👍 for Michal's comment.

erszcz · 2018-05-14T10:27:18Z

I changed escalus_ct:rpc_call to escalus_rpc:call, because the module name changed and stuff needed patching to work. Then I used sed to change escalus_ejabberd:rpc to using distributed_helper & co., but it took quite a while since there were a lot of changes and adding the correct imports and requires was manual (the last commit is still in the pipeline for Travis). I'd like to close this for now.

I think we should gather the remaining points into an issue on Redmine to groom (including @arcusfelis' idea mentioned here: https://github.com/esl/escalus/pull/170/files/29a600038330c08fa8c44ecacf0f501d9710dc35).

michalwski · 2018-05-14T11:22:22Z

I think we should gather the remaining points into an issue on Redmine to groom (including @arcusfelis' idea mentioned here: https://github.com/esl/escalus/pull/170/files/29a600038330c08fa8c44ecacf0f501d9710dc35).

I agree better merge these changes and improve it later.

fenek · 2018-05-15T12:59:42Z

big_tests/tests/distributed_helper.erl

+%%      RPCResult = rpc(mim(), remote_mod, remote_fun, [arg1, arg2]),
+%%      ...
+%%
+require_rpc_nodes(Nodes) ->


👍 for Michal's comment.

fenek · 2018-05-15T16:36:18Z

big_tests/tests/dynamic_modules.erl


 stop(Domain, Mod) ->
    Node = escalus_ct:get_config(ejabberd_node),
    stop(Node, Domain, Mod).

 stop(Node, Domain, Mod) ->
    Cookie = escalus_ct:get_config(ejabberd_cookie),
-    IsLoaded = escalus_ct:rpc_call(Node, gen_mod, is_loaded, [Domain, Mod], 5000, Cookie),
+    IsLoaded = escalus_rpc:call(Node, gen_mod, is_loaded, [Domain, Mod], 5000, Cookie),


👍 for Michal's comment.

fenek · 2018-05-15T16:41:58Z

big_tests/tests/ejabberdctl_SUITE.erl

                    JID = escalus_utils:jid_to_lower(<<Username/binary, "@", Domain/binary >>),
                    lists:any(fun
-                                ([RosterJID, Nick, Sub, "none", Group]) ->
+                                ([RosterJID, __Nick, __Sub, "none", __Group]) ->


I think the aim of this function was to verify that Nick, Sub and Group match the ones from the outer loop so the shadowing should be fixed instead of ignored. :)

I think the aim was to filter the elements with "none".

Shadowing excludes pattern matching, so it couldn't work the way you suggest. For example:

-module(shadow). -compile([export_all]). test() -> L = [A, B] = [1, 2], io:format("~p\n", [lists:filter(fun (A) -> true; (_) -> false end, L)]).

The test shows:

10:22:18 erszcz @ x4 : /tmp $ erlc shadow.erl shadow.erl:2: Warning: export_all flag enabled - all functions will be exported shadow.erl:5: Warning: variable 'A' is unused shadow.erl:5: Warning: variable 'B' is unused shadow.erl:6: Warning: variable 'A' is unused shadow.erl:6: Warning: variable 'A' shadowed in 'fun' shadow.erl:7: Warning: this clause cannot match because a previous clause at line 6 always matches 10:22:20 erszcz @ x4 : /tmp $ erl Erlang/OTP 20 [erts-9.2] [source] [64-bit] [smp:4:4] [ds:4:4:10] [async-threads:10] [kernel-poll:false] Enabled docsh from: /Users/erszcz/work/erszcz/docsh/_build/default/lib/docsh Call h(docsh) for interactive help. Eshell V9.2 (abort with ^G) 1> shadow:test(). [1,2] ok 2>

Ultimately, you're right about the intention (the function was initially introduced in esl/ejabberd_tests@09438c5a#diff-217f8e16b78b791f657c1119ea504f64R519), but I think that since esl/ejabberd_tests@95f4794a#diff-217f8e16b78b791f657c1119ea504f64R670 it doesn't match on Nick, Sub, or Group.

fenek · 2018-05-15T16:54:38Z

big_tests/tests/component_helper.erl

@@ -49,7 +49,7 @@ disconnect_components(Components, Addr) ->
 rpc(M, F, A) ->


Why not distributed_helper:rpc/4?

fenek · 2018-05-15T16:56:29Z

big_tests/tests/ejabberdctl_helper.erl

 ejabberdctl(Cmd, Args, Config) ->
    Node = mim(),
-   ejabberdctl(Node, Cmd, Args, Config).
+    ejabberdctl(Node, Cmd, Args, Config).

 ejabberdctl(Node, Cmd, Args, Config) ->
    CtlCmd = distributed_helper:ctl_path(Node, Config),
    run(string:join([CtlCmd, Cmd | normalize_args(Args)], " ")).

 rpc_call(M, F, Args) ->


Maybe we can deduplicate it with mongoose_helper:successful_rpc/3?

fenek · 2018-05-15T17:02:10Z

big_tests/tests/mam_helper.erl

@@ -950,7 +953,7 @@ parse_messages(Messages) ->

 bootstrap_archive(Config) ->
    random:seed(os:timestamp()),
-    Users = escalus_ct:get_config(escalus_users),
+    _Users = escalus_ct:get_config(escalus_users),


Hmm... so we don't need this line at all, right?

It's obviously a leftover from a debug print/log that somebody's forgotten to remove. I just figured it might be convenient to leave it in case of future debugging, but yes - let's remove it.

fenek · 2018-05-15T17:03:59Z

big_tests/tests/mod_aws_sns_SUITE.erl

@@ -273,7 +273,7 @@ start_publish_listener(Config) ->
 rpc(M, F, A) ->
    Node = ct:get_config({hosts, mim, node}),
    Cookie = escalus_ct:get_config(ejabberd_cookie),
-    escalus_ct:rpc_call(Node, M, F, A, 10000, Cookie).
+    escalus_rpc:call(Node, M, F, A, 10000, Cookie).


Why not distributed_helper:rpc/4?

fenek · 2018-05-15T17:14:26Z

big_tests/tests/mod_global_distrib_SUITE.erl

+wait_for(Label, Pred) ->
+    wait_for(Label, Pred, timer:seconds(5)).
+
+wait_for(Label, _Pred, RemainingTime) when RemainingTime =< 0 ->


Almost duplicate of mongoose_helper:wait_until.

fenek · 2018-05-15T17:23:17Z

big_tests/tests/muc_helper.erl

-                  true -> odbc;
-                  false -> mnesia
-              end,
+    _Backend = case mongoose_helper:is_odbc_enabled(<<"localhost">>) of


Or simply remove _Backend = :)

Same as _Users above.

fenek · 2018-05-15T17:25:32Z

big_tests/tests/push_pubsub_SUITE.erl

@@ -366,7 +366,7 @@ parse_form(Fields) when is_list(Fields) ->
 rpc(M, F, A) ->
    Node = ct:get_config({hosts, mim, node}),
    Cookie = escalus_ct:get_config(ejabberd_cookie),
-    escalus_ct:rpc_call(Node, M, F, A, 10000, Cookie).
+    escalus_rpc:call(Node, M, F, A, 10000, Cookie).


Why not distributed_helper:rpc?

erszcz · 2018-05-16T09:06:59Z

@fenek I've addressed some issues with commits and some with comments. This PR is an extra to the task of decreasing reliance of Escalus on Common Test, not a result of targeted effort to completely clean up RPC in big tests - one step at a time. As I've suggested above we should have a separate issue for complete RPC cleanup, groom it and then resolve as appropriate.

The main pain point is that there's currently an unrelated problem in MUC HTTP API suite which requires like 10 Travis restarts, so I don't want to add anything that requires a CI rerun to this particular PR.

fenek · 2018-05-16T21:05:38Z

I agree that improvement with pinging inside require_rpc_nodes may be done later. Can you please create a ticket for this, so we won't forget to do it?
Same for ejabberdctl_helper:rpc_call
Same for distributed_helper:rpc suggestions
Comment about wait_for is still valid because currently this PR adds a function that is already implemented in even more generic way.

erszcz · 2018-05-17T10:52:48Z

I agree that improvement with pinging inside require_rpc_nodes may be done later. Can you please create a ticket for this, so we won't forget to do it?

I'm keeping my stance that we should not do it.

Same for ejabberdctl_helper:rpc_call
Same for distributed_helper:rpc suggestions

#1853 created.

Comment about wait_for is still valid because currently this PR adds a function that is already implemented in even more generic way.

Apart from mongoose_helper:wait_until/3,4 we also have push_SUITE:wait_for/2 and push_helper:wait_for/2 and a number of ad-hoc wait_for_SOMETHING_SPECIFIC here and there. This is another whole topic which could use some refactoring, so I'd rather not wait a whole day for Travis to recheck this PR if we're going back to this topic in the future. I can create an issue about this, too, so we don't forget.

fenek · 2018-05-17T14:04:15Z

OK, please create a ticket for this as well.

Anyway, I think we need one last Travis confirmation anyway, correct? Last commit has skip ci flag.

erszcz · 2018-05-17T14:21:42Z

Just created #1855

erszcz · 2018-05-17T14:26:45Z

All the commits after 32badd4 have [skip ci], while 32badd4 is green on Travis. There's a minuscule coverage drop, but to be honest I don't know where it comes from - it seems like a spurious change (e.g. different days are hit on a code path which formats dates - o_O).

I think they are simple enough to assume correctness and we'll need to do the job restart magic on master anyway after the PR is merged.

erszcz added 30 commits May 11, 2018 12:46

Fix login_SUITE:init_per_group/2 prerequisite check

398089e

Clean up unused stuff in s2s_SUITE

20163e1

This is a fixup for f8a01ae, which extracted some stuff to `s2s_helper`.

Sketch convenient RPC node access

ebe5860

Commits refactoring the rest of the suites will follow.

Use Escalus with CT-independent RPC call on login_SUITE

b12add1

Use Escalus with escalus_rpc

d9cce90

Do not indent with tabs

c7e9880

Switch from escalus_ejabberd:rpc/3 to rpc/4 in ct_mongoose_hook

40152b4

Switch from escalus_ejabberd:rpc/3 to rpc/4 in ct_tty_hook

1d15a8d

Switch from escalus_ejabberd:rpc/3 to rpc/4 in acc_e2e_SUITE

f20ba62

Switch from escalus_ejabberd:rpc/3 to rpc/4 in amp_big_SUITE

aa13161

Switch from escalus_ejabberd:rpc/3 to rpc/4 in bosh_SUITE

e7a92e4

Switch from escalus_ejabberd:rpc/3 to rpc/4 in conf_reload_SUITE

0c5f324

Switch from escalus_ejabberd:rpc/3 to rpc/4 in connect_SUITE

5e4d5a7

Switch from escalus_ejabberd/3 to rpc/4 in distributed_helper

3bcc45f

Switch from escalus_ejabberd:rpc/3 to rpc/4 in dynamic_modules

6a662c1

Switch from escalus_ejabberd:rpc/3 to rpc/4 in ejabberdctl_SUITE

5fa997a

Fix unused/shadowed variable warnings

0077f04

Switch from escalus_ejabberd:rpc/3 to rpc/4 in ejabberdctl_helper

b37828f

Switch from escalus_ejabberd:rpc/3 to rpc/4 in jingle_SUITE

57262ae

Switch from escalus_ejabberd:rpc/3 to rpc/4 in ldap_helper

539f2cc

Switch from escalus_ejabberd:rpc/3 to rpc/4 in mam_SUITE

88e9ca4

Fix whitespace

ddf9bd3

Fix unused variables in mam_SUITE

494b13e

Switch from escalus_ejabberd:rpc/3 to rpc/4 in mam_helper

ffdf6e1

Fix unused variables in mam_helper

f13e2a7

Switch from escalus_ejabberd:rpc/3 to rpc/4 in metrics_helper

ec8c3be

Switch from escalus_ejabberd:rpc/3 to rpc/4 in metrics_SUITE

5275e82

Switch from escalus_ejabberd:rpc/3 to rpc/4 in metrics_roster_SUITE

72d6596

Switch from escalus_ejabberd:rpc/3 to rpc/4 in mod_http_notification_…

93bcb3c

…SUITE

Switch from escalus_ejabberd:rpc/3 to rpc/4 in mongoose_helper

391ab46

Try rebalancing until it succeeds

3e20aa6

erszcz requested a review from kzemek May 14, 2018 07:10

erszcz added 3 commits May 14, 2018 10:18

Use distributed_helper:mim{2,3}/0

a86236c

Fix whitespace and remove an unused macro

386382d

Remove redundancy between ditributed_helper and ejabberd_node_utils

2d8b36e

michalwski reviewed May 14, 2018

View reviewed changes

Fix RPC node references now removed from ejabberd_node_utils

32badd4

fenek requested changes May 15, 2018

View reviewed changes

erszcz added 2 commits May 16, 2018 10:53

[skip ci] Remove unused variables altogether

5f64475

[skip ci] Use a direct call to an imported function

8965697

[skip ci] Revert changes to ejabberdctl_SUITE:match_roster/2

3e3be2e

fenek added the WIP 🚧 label May 16, 2018

erszcz mentioned this pull request May 17, 2018

RPC cleanup in big tests #1853

Closed

fenek approved these changes May 17, 2018

View reviewed changes

kzemek closed this May 17, 2018

kzemek reopened this May 17, 2018

kzemek merged commit af131fd into master May 17, 2018

kzemek deleted the rpc-cleanup branch May 17, 2018 14:32

kzemek removed the WIP 🚧 label May 17, 2018

fenek added this to the 3.0.0 milestone May 22, 2018

erszcz mentioned this pull request May 23, 2018

Fix some unstable tests #1874

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Partial RPC cleanup in big tests #1846

Partial RPC cleanup in big tests #1846

erszcz commented May 11, 2018 •

edited

Loading

codecov bot commented May 11, 2018 •

edited

Loading

michalwski left a comment

michalwski May 14, 2018

michalwski May 14, 2018

fenek May 15, 2018 •

edited

Loading

erszcz May 16, 2018

michalwski May 14, 2018

fenek May 15, 2018 •

edited

Loading

erszcz commented May 14, 2018 •

edited

Loading

michalwski commented May 14, 2018

fenek May 15, 2018 •

edited

Loading

fenek May 15, 2018 •

edited

Loading

fenek May 15, 2018

erszcz May 16, 2018

erszcz May 16, 2018 •

edited

Loading

fenek May 15, 2018

fenek May 15, 2018

fenek May 15, 2018

erszcz May 16, 2018

fenek May 15, 2018

fenek May 15, 2018

fenek May 15, 2018

erszcz May 16, 2018

fenek May 15, 2018

erszcz commented May 16, 2018 •

edited

Loading

fenek commented May 16, 2018

erszcz commented May 17, 2018 •

edited

Loading

fenek commented May 17, 2018

erszcz commented May 17, 2018

erszcz commented May 17, 2018 •

edited

Loading

		@@ -49,7 +49,7 @@ disconnect_components(Components, Addr) ->
		rpc(M, F, A) ->

Partial RPC cleanup in big tests #1846

Partial RPC cleanup in big tests #1846

Conversation

erszcz commented May 11, 2018 • edited Loading

codecov bot commented May 11, 2018 • edited Loading

Codecov Report

michalwski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fenek May 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fenek May 15, 2018 • edited Loading

Choose a reason for hiding this comment

erszcz commented May 14, 2018 • edited Loading

michalwski commented May 14, 2018

fenek May 15, 2018 • edited Loading

Choose a reason for hiding this comment

fenek May 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erszcz May 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erszcz commented May 16, 2018 • edited Loading

fenek commented May 16, 2018

erszcz commented May 17, 2018 • edited Loading

fenek commented May 17, 2018

erszcz commented May 17, 2018

erszcz commented May 17, 2018 • edited Loading

erszcz commented May 11, 2018 •

edited

Loading

codecov bot commented May 11, 2018 •

edited

Loading

fenek May 15, 2018 •

edited

Loading

fenek May 15, 2018 •

edited

Loading

erszcz commented May 14, 2018 •

edited

Loading

fenek May 15, 2018 •

edited

Loading

fenek May 15, 2018 •

edited

Loading

erszcz May 16, 2018 •

edited

Loading

erszcz commented May 16, 2018 •

edited

Loading

erszcz commented May 17, 2018 •

edited

Loading

erszcz commented May 17, 2018 •

edited

Loading