Missing domain admin tests #3745

jacekwegr · 2022-09-02T13:04:00Z

This PR adds missing domain admin tests for GraphQL api

codecov · 2022-09-02T13:13:54Z

Codecov Report

Base: 82.73% // Head: 82.69% // Decreases project coverage by -0.03% ⚠️

Coverage data is based on head (82aa71f) compared to base (6b39a79).
Patch has no changes to coverable lines.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3745      +/-   ##
==========================================
- Coverage   82.73%   82.69%   -0.04%     
==========================================
  Files         529      529              
  Lines       33957    33957              
==========================================
- Hits        28094    28081      -13     
- Misses       5863     5876      +13

Impacted Files	Coverage Δ
src/mongoose_account_api.erl	`94.52% <0.00%> (-2.74%)`	⬇️
src/mod_roster_riak.erl	`96.92% <0.00%> (-1.54%)`	⬇️
src/logger/mongoose_log_filter.erl	`78.08% <0.00%> (-1.37%)`	⬇️
src/rdbms/mongoose_rdbms.erl	`65.26% <0.00%> (-1.06%)`	⬇️
src/mod_roster.erl	`78.70% <0.00%> (-0.72%)`	⬇️
src/ejabberd_c2s.erl	`88.93% <0.00%> (-0.38%)`	⬇️
src/mod_muc_room.erl	`78.22% <0.00%> (-0.18%)`	⬇️
src/mod_muc_log.erl	`62.82% <0.00%> (ø)`
src/inbox/mod_inbox_rdbms_async.erl	`72.05% <0.00%> (ø)`
src/mam/mod_mam_elasticsearch_arch.erl	`88.39% <0.00%> (+1.78%)`	⬆️
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

JanuszJakubiec · 2022-09-08T10:45:56Z

big_tests/tests/graphql_gdpr_SUITE.erl

@@ -68,6 +77,16 @@ admin_gdpr_no_user_test(Config) ->
    Res = admin_retrieve_personal_data(<<"AAAA">>, domain(), <<"AAA">>, Config),
    ?assertEqual(<<"user_does_not_exist_error">>, get_err_code(Res)).

+domain_admin_retrive_user_data_no_permission(Config) ->
+    escalus:fresh_story_with_config(Config, [{alice_bis, 1}], 


Suggested change

escalus:fresh_story_with_config(Config, [{alice_bis, 1}],

escalus:fresh_story_with_config(Config, [{alice_bis, 1}],

big_tests/tests/graphql_helper.erl

JanuszJakubiec · 2022-09-08T10:56:10Z

big_tests/tests/graphql_inbox_SUITE.erl

+domain_admin_flush_global_bin_no_permission(Config, Alice, AliceBis, Kate) ->
+    SecHostType = domain_helper:host_type(),
+    create_room_and_make_users_leave(Alice, AliceBis, Kate),
+    get_unauthorized(flush_global_bin(SecHostType, null, Config)). % check it


I suggest deleting this comment.

JanuszJakubiec · 2022-09-08T10:58:57Z

big_tests/tests/graphql_last_SUITE.erl

+    get_unauthorized(admin_remove_old_users(<<"external">>, now_dt_with_offset(0), Config)).
+
+domain_admin_list_old_users_global(Config) ->
+    jids_with_config(Config, [alice, alice_bis, bob], 


Suggested change

jids_with_config(Config, [alice, alice_bis, bob],

jids_with_config(Config, [alice, alice_bis, bob],

JanuszJakubiec · 2022-09-08T10:59:21Z

big_tests/tests/graphql_last_SUITE.erl

+    get_unauthorized(admin_list_old_users(null, now_dt_with_offset(150), Config)).
+
+domain_admin_remove_old_users_global(Config) ->
+    jids_with_config(Config, [alice, alice_bis, bob], 


Suggested change

jids_with_config(Config, [alice, alice_bis, bob],

jids_with_config(Config, [alice, alice_bis, bob],

JanuszJakubiec · 2022-09-08T11:46:25Z

big_tests/tests/graphql_session_SUITE.erl

+    case Config1 of
+        {skip, require_rdbms} -> 
+            Config1;
+        _ -> 


Suggested change

_ ->

_ ->

JanuszJakubiec · 2022-09-08T11:46:34Z

big_tests/tests/graphql_session_SUITE.erl

+init_per_group(domain_admin_session, Config) ->
+    Config1 = graphql_helper:init_domain_admin_handler(Config),
+    case Config1 of
+        {skip, require_rdbms} -> 


Suggested change

{skip, require_rdbms} ->

{skip, require_rdbms} ->

JanuszJakubiec

Looks good, I've added some minor stylistic comments

JanuszJakubiec

Looks good

chrzaszcz

It is going in the right direction in general. However, I think that in many cases the tests are not in line with the main goal - to check that the domain admin can't access a different (existing) domain - which is the main purpose of the domain permission level.

So, on one hand I see that we are repeating all tests for non-existent domains (which is good, but I don't think it's necessary), but on the other hand sometimes we miss the most important test: for the secondary domain - either with domain_helper:secondary_domain(), or with a user like AliceBis. I didn't mark all such places, because some tests (e.g. muc and muc_light) are missing such tests entirely.

big_tests/tests/domain_helper.erl

big_tests/tests/graphql_account_SUITE.erl

big_tests/tests/graphql_muc_light_SUITE.erl

big_tests/tests/graphql_token_SUITE.erl

big_tests/tests/graphql_vcard_SUITE.erl

big_tests/tests/graphql_offline_SUITE.erl

big_tests/tests/graphql_private_SUITE.erl

chrzaszcz

A lot of good tests here. I added some comments, mostly minor ones.

big_tests/tests/graphql_gdpr_SUITE.erl

big_tests/tests/graphql_last_SUITE.erl

big_tests/tests/graphql_muc_SUITE.erl

big_tests/tests/graphql_muc_light_SUITE.erl

big_tests/tests/graphql_token_SUITE.erl

big_tests/tests/graphql_offline_SUITE.erl

big_tests/tests/graphql_private_SUITE.erl

big_tests/tests/graphql_stanza_SUITE.erl

mongoose-im · 2022-09-20T08:30:02Z

small_tests_24 / small_tests / 82aa71f
Reports root / small

small_tests_25 / small_tests / 82aa71f
Reports root / small

ldap_mnesia_24 / ldap_mnesia / 82aa71f
Reports root/ big
OK: 1972 / Failed: 0 / User-skipped: 716 / Auto-skipped: 0

dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / 82aa71f
Reports root/ big
OK: 3780 / Failed: 0 / User-skipped: 88 / Auto-skipped: 0

dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / 82aa71f
Reports root/ big
OK: 3780 / Failed: 0 / User-skipped: 88 / Auto-skipped: 0

ldap_mnesia_25 / ldap_mnesia / 82aa71f
Reports root/ big
OK: 1972 / Failed: 0 / User-skipped: 716 / Auto-skipped: 0

dynamic_domains_mysql_redis_25 / mysql_redis / 82aa71f
Reports root/ big
OK: 3763 / Failed: 0 / User-skipped: 105 / Auto-skipped: 0

internal_mnesia_25 / internal_mnesia / 82aa71f
Reports root/ big
OK: 2093 / Failed: 0 / User-skipped: 595 / Auto-skipped: 0

dynamic_domains_mssql_mnesia_25 / odbc_mssql_mnesia / 82aa71f
Reports root/ big
OK: 3780 / Failed: 0 / User-skipped: 88 / Auto-skipped: 0

elasticsearch_and_cassandra_25 / elasticsearch_and_cassandra_mnesia / 82aa71f
Reports root/ big
OK: 2433 / Failed: 0 / User-skipped: 590 / Auto-skipped: 0

pgsql_mnesia_24 / pgsql_mnesia / 82aa71f
Reports root/ big
OK: 4154 / Failed: 0 / User-skipped: 97 / Auto-skipped: 0

mysql_redis_25 / mysql_redis / 82aa71f
Reports root/ big
OK: 4149 / Failed: 0 / User-skipped: 102 / Auto-skipped: 0

pgsql_mnesia_25 / pgsql_mnesia / 82aa71f
Reports root/ big
OK: 4154 / Failed: 0 / User-skipped: 97 / Auto-skipped: 0

mssql_mnesia_25 / odbc_mssql_mnesia / 82aa71f
Reports root/ big
OK: 4154 / Failed: 0 / User-skipped: 97 / Auto-skipped: 0

riak_mnesia_24 / riak_mnesia / 82aa71f
Reports root/ big
OK: 2272 / Failed: 0 / User-skipped: 583 / Auto-skipped: 0

chrzaszcz

All looks good now 👌

This comment was marked as outdated.

Sign in to view

mongoose-im mentioned this pull request Sep 2, 2022

CI build failures summary - part 2 #3685

Closed

jacekwegr force-pushed the graphql-domain-admin-tests branch from c4082d9 to c96b005 Compare September 5, 2022 11:30

This comment was marked as outdated.

Sign in to view

jacekwegr force-pushed the graphql-domain-admin-tests branch from c96b005 to 687cd1e Compare September 5, 2022 12:44

This comment was marked as outdated.

Sign in to view

jacekwegr force-pushed the graphql-domain-admin-tests branch from 687cd1e to d31c2cc Compare September 5, 2022 14:33

This comment was marked as outdated.

Sign in to view

jacekwegr force-pushed the graphql-domain-admin-tests branch from d31c2cc to fe0defa Compare September 6, 2022 07:29

This comment was marked as outdated.

Sign in to view

jacekwegr force-pushed the graphql-domain-admin-tests branch from fe0defa to d88944b Compare September 6, 2022 14:14

This comment was marked as outdated.

Sign in to view

jacekwegr force-pushed the graphql-domain-admin-tests branch from d88944b to e3494a7 Compare September 6, 2022 14:58

This comment was marked as outdated.

Sign in to view

jacekwegr force-pushed the graphql-domain-admin-tests branch from e3494a7 to ab5748e Compare September 7, 2022 07:34

This comment was marked as outdated.

Sign in to view

jacekwegr marked this pull request as ready for review September 7, 2022 08:25

JanuszJakubiec reviewed Sep 8, 2022

View reviewed changes

big_tests/tests/graphql_helper.erl Show resolved Hide resolved

JanuszJakubiec reviewed Sep 8, 2022

View reviewed changes

Missing domain admin tests

f84b41b

jacekwegr force-pushed the graphql-domain-admin-tests branch from ab5748e to f84b41b Compare September 8, 2022 13:49

This comment was marked as outdated.

Sign in to view

JanuszJakubiec approved these changes Sep 8, 2022

View reviewed changes

chrzaszcz reviewed Sep 9, 2022

View reviewed changes

This comment was marked as outdated.

Sign in to view

chrzaszcz reviewed Sep 19, 2022

View reviewed changes

jacekwegr force-pushed the graphql-domain-admin-tests branch from cc3202f to 8f3ea9a Compare September 19, 2022 14:27

This comment was marked as outdated.

Sign in to view

Domain admin tests focused on existing domains

82aa71f

jacekwegr force-pushed the graphql-domain-admin-tests branch from 8f3ea9a to 82aa71f Compare September 20, 2022 08:21

chrzaszcz approved these changes Sep 20, 2022

View reviewed changes

chrzaszcz merged commit ee45d84 into master Sep 20, 2022

chrzaszcz deleted the graphql-domain-admin-tests branch September 20, 2022 12:00

chrzaszcz added this to the 6.0.0 milestone Dec 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Missing domain admin tests #3745

Missing domain admin tests #3745

jacekwegr commented Sep 2, 2022

This comment was marked as outdated.

codecov bot commented Sep 2, 2022 •

edited

Loading

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

JanuszJakubiec Sep 8, 2022

JanuszJakubiec Sep 8, 2022 •

edited

Loading

JanuszJakubiec Sep 8, 2022

JanuszJakubiec Sep 8, 2022

JanuszJakubiec Sep 8, 2022

JanuszJakubiec Sep 8, 2022

JanuszJakubiec left a comment

This comment was marked as outdated.

JanuszJakubiec left a comment

chrzaszcz left a comment •

edited

Loading

This comment was marked as outdated.

chrzaszcz left a comment

This comment was marked as outdated.

mongoose-im commented Sep 20, 2022 •

edited

Loading

chrzaszcz left a comment

	escalus:fresh_story_with_config(Config, [{alice_bis, 1}],
	escalus:fresh_story_with_config(Config, [{alice_bis, 1}],

	jids_with_config(Config, [alice, alice_bis, bob],
	jids_with_config(Config, [alice, alice_bis, bob],

Missing domain admin tests #3745

Missing domain admin tests #3745

Conversation

jacekwegr commented Sep 2, 2022

This comment was marked as outdated.

codecov bot commented Sep 2, 2022 • edited Loading

Codecov Report

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

JanuszJakubiec Sep 8, 2022

Choose a reason for hiding this comment

JanuszJakubiec Sep 8, 2022 • edited Loading

Choose a reason for hiding this comment

JanuszJakubiec Sep 8, 2022

Choose a reason for hiding this comment

JanuszJakubiec Sep 8, 2022

Choose a reason for hiding this comment

JanuszJakubiec Sep 8, 2022

Choose a reason for hiding this comment

JanuszJakubiec Sep 8, 2022

Choose a reason for hiding this comment

JanuszJakubiec left a comment

Choose a reason for hiding this comment

This comment was marked as outdated.

JanuszJakubiec left a comment

Choose a reason for hiding this comment

chrzaszcz left a comment • edited Loading

Choose a reason for hiding this comment

This comment was marked as outdated.

chrzaszcz left a comment

Choose a reason for hiding this comment

This comment was marked as outdated.

mongoose-im commented Sep 20, 2022 • edited Loading

chrzaszcz left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 2, 2022 •

edited

Loading

JanuszJakubiec Sep 8, 2022 •

edited

Loading

chrzaszcz left a comment •

edited

Loading

mongoose-im commented Sep 20, 2022 •

edited

Loading