unit tests for mongoose_lazy_routing module #3127

DenysGonchar · 2021-05-24T23:33:27Z

This PR introduces unit tests for mongoose_lazy_routing module

…ation

codecov · 2021-05-25T07:52:34Z

Codecov Report

Merging #3127 (744e0d4) into master (eae2da0) will decrease coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3127      +/-   ##
==========================================
- Coverage   79.31%   79.25%   -0.06%     
==========================================
  Files         394      394              
  Lines       32150    32155       +5     
==========================================
- Hits        25500    25485      -15     
- Misses       6650     6670      +20

Impacted Files	Coverage Δ
src/domain/mongoose_lazy_routing.erl	`87.80% <100.00%> (+4.61%)`	⬆️
src/domain/mongoose_subdomain_core.erl	`89.52% <100.00%> (+0.10%)`	⬆️
src/elasticsearch/mongoose_elasticsearch.erl	`76.92% <0.00%> (-7.70%)`	⬇️
src/ejabberd_s2s_out.erl	`58.81% <0.00%> (-2.98%)`	⬇️
src/cassandra/mongoose_cassandra_worker.erl	`70.75% <0.00%> (-2.36%)`	⬇️
src/auth/ejabberd_auth_internal.erl	`85.57% <0.00%> (-1.93%)`	⬇️
src/mam/mod_mam_elasticsearch_arch.erl	`86.60% <0.00%> (-1.79%)`	⬇️
src/auth/ejabberd_auth.erl	`75.80% <0.00%> (-1.08%)`	⬇️
src/ejabberd_admin.erl	`59.24% <0.00%> (-0.95%)`	⬇️
src/ejabberd_local.erl	`77.51% <0.00%> (-0.78%)`	⬇️
... and 10 more

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 eae2da0...744e0d4. Read the comment docs.

arcusfelis

looks fine, I almost understand the tests.
Added comments about whitespaces though.

test/mongoose_subdomain_core_SUITE.erl

test/mongoose_lazy_routing_SUITE.erl

arcusfelis · 2021-05-25T10:13:58Z

test/mongoose_lazy_routing_SUITE.erl

+    ?assertEqual([], get_all_unregistered_iqs()),
+    meck:reset(gen_iq_component),
+    %% register 2 IQ handlers for ?HOST_TYPE_1 subdomains, then add one subdomain and
+    %% one domain (just to ensure that domain doesn't affect anything)


I prefer tests to follow this convention:

can_register_iq_handler_for_subdomain() -> %% GIVEN idk, for me nothing is given in this case %% WHEN Register IQ handler for subdomain ?assertEqual(true, maybe_add_domain_or_subdomain(?SUBDOMAIN_2)), %% ONE subdomain %% THEN registered assert_subdomain_registered(?SUBDOMAIN_2).

If we want to test registering two subdomains, we do another case:

can_register_iq_handler_for_two_subdomains() -> %% GIVEN idk, for me nothing is given in this case %% WHEN Register IQ handler for subdomains ?assertEqual(true, maybe_add_domain_or_subdomain(?SUBDOMAIN_1)), ?assertEqual(true, maybe_add_domain_or_subdomain(?SUBDOMAIN_2)), %% THEN registered assert_subdomain_registered(?SUBDOMAIN_1), assert_subdomain_registered(?SUBDOMAIN_2).

If we want to test unregister:

can_unregister_iq_handler_for_two_subdomains() -> %% GIVEN two registered domains ?assertEqual(true, maybe_add_domain_or_subdomain(?SUBDOMAIN_1)), ?assertEqual(true, maybe_add_domain_or_subdomain(?SUBDOMAIN_2)), %% WHEN Register IQ handler for subdomains unregister_subdomain(?SUBDOMAIN_1), unregister_subdomain(?SUBDOMAIN_2), %% THEN registered assert_subdomain_not_registered(?SUBDOMAIN_1), assert_subdomain_not_registered(?SUBDOMAIN_2).

I am not a big fun of asserts in the given part, so:

can_unregister_iq_handler_for_two_subdomains() -> %% GIVEN two registered domains given_subdomains([?SUBDOMAIN_1, ?SUBDOMAIN_2]), %% WHEN Unegister IQ handler for subdomains unregister_subdomain(?SUBDOMAIN_1), unregister_subdomain(?SUBDOMAIN_2), %% THEN not registered assert_subdomain_not_registered(?SUBDOMAIN_1), assert_subdomain_not_registered(?SUBDOMAIN_2).

If you check more than two things in a case, it is still ok, unless test fails. After that it's hard to understand what is actually failing.
If you do something that is not described in test name - it's very hard to understand if the test is valid or not.

Overall, it's a good idea to follow GIVEN... WHEN... THAN... structure.
And make helper functions for GIVEN and THAN conditions.

And generally, it's good idea to think about post conditions. They are our side effects. What does it mean subdomain not registered? It could be encapsulated inside that assert_subdomain_not_registered function.

Side effects dictate the name of the test case. Action (WHEN block) in the test is the secondary, but also should be present in the name.

%% Was can_unregister_iq_handler_for_two_subdomains unregistered_subdomain_is_not_listed() -> %% GIVEN given_subdomains([?SUBDOMAIN_1]), %% WHEN unregister_subdomain(?SUBDOMAIN_1), %% THEN not registered assert_subdomain_not_registered(?SUBDOMAIN_1). unregistered_subdomain_looses_iq_handlers() -> %% GIVEN given_subdomains([?SUBDOMAIN_1]), given_iq_handler(...), %% WHEN unregister_subdomain(?SUBDOMAIN_1), %% THEN not registered assert_iq_handler_not_registered(...).

Also, testing with more complex preconditions is cool, but harder to read. Still can be done, but in a separate test case.

%% Was can_unregister_iq_handler_for_two_subdomains unregistered_subdomain_is_not_listed_when_there_is_another_domain() -> %% GIVEN given_subdomains([?SUBDOMAIN_2]), unregistered_subdomain_looses_iq_handlers().

I agree that splitting this huge test case could help with readability.
Personally I think that naming functions given_* is not a good practice - I'd rather keep the namesregister_subdomains, register_iq_handler etc. to be explicit about what they are exactly doing.
IMO there is no need for a significant rework here, splitting could help (test sequence in a group could suffice), but it's not a must.

given, when, then template doesn't fit well for the cases that test correct setup/correct rollback behaviour.
I will try to split can_register_and_unregister_iq_handler_for_domain and can_register_and_unregister_iq_handler_for_subdomain test case into 4 smaller

chrzaszcz

Looks good! I have a few minor comments.

test/mongoose_lazy_routing_SUITE.erl

chrzaszcz · 2021-05-25T12:04:55Z

test/mongoose_lazy_routing_SUITE.erl

+    ?assertEqual([], get_all_unregistered_iqs()),
+    meck:reset(gen_iq_component),
+    %% register 2 IQ handlers for ?HOST_TYPE_1 subdomains, then add one subdomain and
+    %% one domain (just to ensure that domain doesn't affect anything)


I agree that splitting this huge test case could help with readability.
Personally I think that naming functions given_* is not a good practice - I'd rather keep the namesregister_subdomains, register_iq_handler etc. to be explicit about what they are exactly doing.
IMO there is no need for a significant rework here, splitting could help (test sequence in a group could suffice), but it's not a must.

chrzaszcz

Looks good!

chrzaszcz · 2021-05-26T07:31:18Z

test/mongoose_lazy_routing_SUITE.erl

+    ?assertEqual([], get_all_registered_iqs()),
+    ?assertEqual([], get_all_unregistered_iqs()).
+
+


Minor: double empty line.

DenysGonchar added 6 commits May 21, 2021 21:43

extending checks at mongoose_domain_core_SUITE

73405cb

extending mongoose_subdomain_core_SUITE tests

5e4d8f5

fixing handle_unregister_iq_handler_for_subdomain

d77b3e3

disabling subdomain in case of conflict with top level domain

cc49b20

fixing removal of the records from ?IQ_TABLE on IQ handlers deregistr…

e714cdb

…ation

adding mongoose_lazy_routing_SUITE

5153cc7

DenysGonchar requested review from chrzaszcz and arcusfelis May 24, 2021 23:33

arcusfelis reviewed May 25, 2021

View reviewed changes

chrzaszcz reviewed May 25, 2021

View reviewed changes

DenysGonchar added 2 commits May 25, 2021 18:21

minor changes according to the code review comments

bb0357e

splitting test cases at mongoose_lazy_routing_SUITE

744e0d4

DenysGonchar force-pushed the mongoose_lazy_routing-unit-tests branch from a28d8eb to 744e0d4 Compare May 25, 2021 16:21

chrzaszcz approved these changes May 26, 2021

View reviewed changes

chrzaszcz merged commit 1ecab49 into master May 26, 2021

chrzaszcz deleted the mongoose_lazy_routing-unit-tests branch May 26, 2021 07:34

Premwoik added this to the 5.0.0 milestone Oct 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

unit tests for mongoose_lazy_routing module #3127

unit tests for mongoose_lazy_routing module #3127

DenysGonchar commented May 24, 2021

codecov bot commented May 25, 2021 •

edited

arcusfelis left a comment

arcusfelis May 25, 2021

chrzaszcz May 25, 2021

DenysGonchar May 25, 2021

chrzaszcz left a comment

chrzaszcz May 25, 2021

chrzaszcz left a comment

chrzaszcz May 26, 2021

		?assertEqual([], get_all_registered_iqs()),
		?assertEqual([], get_all_unregistered_iqs()).

unit tests for mongoose_lazy_routing module #3127

unit tests for mongoose_lazy_routing module #3127

Conversation

DenysGonchar commented May 24, 2021

codecov bot commented May 25, 2021 • edited

Codecov Report

arcusfelis left a comment

Choose a reason for hiding this comment

arcusfelis May 25, 2021

Choose a reason for hiding this comment

chrzaszcz May 25, 2021

Choose a reason for hiding this comment

DenysGonchar May 25, 2021

Choose a reason for hiding this comment

chrzaszcz left a comment

Choose a reason for hiding this comment

chrzaszcz May 25, 2021

Choose a reason for hiding this comment

chrzaszcz left a comment

Choose a reason for hiding this comment

chrzaszcz May 26, 2021

Choose a reason for hiding this comment

codecov bot commented May 25, 2021 •

edited