Skip to content
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

muc_SUITE kicked_after_sending_malformed_presence test #1692

Merged
merged 6 commits into from
Feb 1, 2018

Conversation

SauronSauron
Copy link
Contributor

@SauronSauron SauronSauron commented Jan 29, 2018

muc_SUITE kicked_after_sending_malformed_presence does not terminate room until test is done.

This PR addresses redmine 17047

17047 Description

I'm disabling this test case on master to reduce the pain with Travis builds. It has to be verified and fixed.

====== Suite FAILED: muc_SUITE (1 of 119 tests failed)
====== Test name: kicked_after_sending_malformed_presence
====== Reason:    {timeout_when_waiting_for_stanza,
                      [{escalus_client,wait_for_stanza,
                           [{client,<<"kate87.108954@localhost/res1">>,
                                escalus_tcp,#Port<0.40135>,false,false,
                                <0.20540.0>,
                                [{event_manager,<0.20285.0>},
                                 {server,<<"localhost">>},
                                 {username,<<"kate87.108954">>},
                                 {resource,<<"res1">>}]},
                            1000],
                           [{file,"src/escalus_client.erl"},{line,121}]},
                       {muc_SUITE,
                           '-kicked_after_sending_malformed_presence/1-fun-0-',
                           3,
                           [{file,"muc_SUITE.erl"},{line,2721}]},
                       {escalus_story,story,3,
                           [{file,"src/escalus_story.erl"},{line,40}]},
                       {muc_SUITE,kicked_after_sending_malformed_presence,1,
                           [{file,"muc_SUITE.erl"},{line,2698}]},
                       {test_server,ts_tc,3,
                           [{file,"test_server.erl"},{line,1533}]},
                       {test_server,run_test_case_eval1,6,
                           [{file,"test_server.erl"},{line,1053}]},
                       {test_server,run_test_case_eval,9,
                           [{file,"test_server.erl"},{line,985}]}]}

@SauronSauron
Copy link
Contributor Author

I copied the module from another branch and now it seems as if I changed the whole module.
The changes I have done is between line 2792 - 2823. You want me to delete this PR and redo it?

@fenek
Copy link
Member

fenek commented Jan 30, 2018

Please simply force push to this branch, no need to create new PR.
Next time I'd recommend using git checkout branch -- file to "copy" the file, not copy & paste. When you take a closer look - you've created ~4000 indent changes.

escalus:assert(is_presence_with_type, [<<"unavailable">>], PresenceUn),
escalus:assert(is_stanza_from, [room_address(Room, Username)], PresenceUn),
escalus:wait_for_stanza(Bob),
escalus:assert(is_presence_with_type, [<<"unavailable">>], BobStanza = escalus:wait_for_stanza(Bob)),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assignment inside function calls is not allowed by code standards (?).

%% GIVEN An user is in the room
Room = ?config(room, Config),
KateUsername = escalus_utils:get_username(Kate),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need Kate in the test?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To prevent room process from terminating when Bob is kicked

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, maybe the whole test should be something like:

kicked_after_sending_malformed_presence(Config) ->
      muc_helper:with_fresh_room(Config, [{alice, 1}], fun(RoomName, Alice) ->
             muc_helper:join(RoomName, Alice),
             muc_helper:send_malformed_presence(RoomName, Alice),
             muc_helper:gets_kicked(RoomName, Alice),
             muc_helper:assert_not_in_the_room(RoomName, Alice)
             end).

:)
(this comment should be ignored for this PR).

kicked_after_sending_malformed_presence(Config1) ->
AliceSpec = given_fresh_spec(Config1, alice),
Config = given_fresh_room(Config1, AliceSpec, []),
escalus:fresh_story(Config, [{bob, 1}, {kate, 1},{alice, 1}], fun(Bob, Kate, Alice) ->
%% GIVEN An user is in the room
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we keep both a comment before the testcase and these small comments for each operation?

escalus:fresh_story(Config, [{bob, 1}, {kate, 1}], fun(Bob, Kate) ->
%% GIVEN An user is in the room
escalus:fresh_story(Config, [{bob, 1}, {kate, 1},{alice, 1}], fun(Bob, Kate, Alice) ->
%GIVEN Kate in the room, to ensure that the room does not get stopped
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indention ><

Copy link
Contributor

@arcusfelis arcusfelis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merge, but squish, replace comments.
Add info about why and how it got fixed into merge comment.

@SauronSauron SauronSauron merged commit 0c5c6c4 into master Feb 1, 2018
@SauronSauron SauronSauron deleted the muc-suite-kicked branch February 1, 2018 14:39
@fenek fenek added this to the 3.0.0 milestone Feb 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants