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

Pass local_host option when accepting session #1958

Merged
merged 1 commit into from
Jul 4, 2018

Conversation

arkgil
Copy link
Contributor

@arkgil arkgil commented Jul 3, 2018

Lack of this option made nksip generate wrong Record-Route header.

@michalwski Do you have an idea how to test such change? Does esip allow to provide some kind of callbacks which would be called when SIP stanzas reach its endpoint?

Copy link
Contributor

@michalwski michalwski left a comment

Choose a reason for hiding this comment

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

Thanks for the change. As for testing if I understand and remember correctly, this reply results in a 200 OK SIP message sent from Mongoose to the Proxy. If so then there are callbacks for it already. It's all in big_tests/tests/jingle_sip_translator.erl, most probably it will be in_invite_transaction_callback/4, other place would be back_invite_callbacks.

%case nksip_request:reply({ok, [{body, SDP}]}, ReqID) of
case nksip_request_reply({ok, [{body, SDP}]}, ReqID) of
LocalHost = gen_mod:get_module_opt(Server, ?MODULE, local_host, "localhost"),
%case nksip_request:reply({ok, [{body, SDP}, {local_host, LocalHost}]}, ReqID) of
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the commented line can be removed.

@arkgil
Copy link
Contributor Author

arkgil commented Jul 3, 2018

@michalwski Now that I think about it, asserting Record-Route header in tests won't do any good - it was set to "localhost" anyway, because nksip defaults to address of proxy.

Lack of this option made nksip generate wrong Record-Route header.
@mongoose-im
Copy link
Collaborator

mongoose-im commented Jul 3, 2018

5033.1 / Erlang 19.3 / small_tests / 49c5524
Reports root / small


5033.2 / Erlang 19.3 / internal_mnesia / 49c5524
Reports root/ big
OK: 1060 / Failed: 0 / User-skipped: 41 / Auto-skipped: 0


5033.6 / Erlang 19.3 / elasticsearch_and_cassandra_mnesia / 49c5524
Reports root/ big
OK: 445 / Failed: 0 / User-skipped: 8 / Auto-skipped: 0


5033.3 / Erlang 19.3 / mysql_redis / 49c5524
Reports root/ big
OK: 2799 / Failed: 0 / User-skipped: 222 / Auto-skipped: 0


5033.5 / Erlang 19.3 / ldap_mnesia / 49c5524
Reports root/ big
OK: 1024 / Failed: 0 / User-skipped: 77 / Auto-skipped: 0


5033.4 / Erlang 19.3 / odbc_mssql_mnesia / 49c5524
Reports root/ big
OK: 2794 / Failed: 0 / User-skipped: 209 / Auto-skipped: 0


5033.8 / Erlang 20.0 / pgsql_mnesia / 49c5524
Reports root/ big / small
OK: 2845 / Failed: 0 / User-skipped: 176 / Auto-skipped: 0


5033.9 / Erlang 21.0 / riak_mnesia / 49c5524
Reports root/ big / small
OK: 1279 / Failed: 0 / User-skipped: 43 / Auto-skipped: 0

@codecov
Copy link

codecov bot commented Jul 3, 2018

Codecov Report

Merging #1958 into master will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1958      +/-   ##
==========================================
+ Coverage   74.75%   74.81%   +0.05%     
==========================================
  Files         309      309              
  Lines       28047    28048       +1     
==========================================
+ Hits        20966    20983      +17     
+ Misses       7081     7065      -16
Impacted Files Coverage Δ
src/jingle_sip/mod_jingle_sip.erl 81.02% <100%> (+0.07%) ⬆️
..._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/mam/mod_mam_odbc_prefs.erl 92.52% <0%> (-3.74%) ⬇️
src/rdbms/mongoose_rdbms.erl 67.83% <0%> (-2.52%) ⬇️
src/mod_bosh.erl 93.27% <0%> (-1.69%) ⬇️
src/mod_bosh_socket.erl 80.66% <0%> (-1%) ⬇️
src/mam/mod_mam_utils.erl 82.37% <0%> (-0.68%) ⬇️
src/mod_muc.erl 72.2% <0%> (-0.28%) ⬇️
src/pubsub/mod_pubsub.erl 65.99% <0%> (+0.06%) ⬆️
... and 8 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 984dd6d...09f72a8. Read the comment docs.

@michalwski michalwski merged commit 9209557 into master Jul 4, 2018
@michalwski michalwski deleted the sip-reply-local-host branch July 4, 2018 07:24
@fenek fenek added this to the 3.0.0++ milestone Jul 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants