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

Add UCS-2 support to SMS API #1410

Merged
merged 17 commits into from Sep 28, 2016

Conversation

Projects
None yet
4 participants
@alexclear
Contributor

alexclear commented Sep 27, 2016

No description provided.

@otsakir otsakir added the Peer Review label Sep 27, 2016

@deruelle deruelle added this to the 8.0.0 milestone Sep 27, 2016

@deruelle

This comment has been minimized.

Member

deruelle commented Sep 27, 2016

Thanks @alexclear. @gvagenas can you review ?

@gvagenas

This comment has been minimized.

Collaborator

gvagenas commented Sep 27, 2016

@alexclear Thanks for your PR and your work to support UCS-2 encoding for sms.

I checked your work and looks good to me but there no test cases provided in the PR in order to test the encoding, both GSM and UCS-2, can you please add some for both outgoing and incoming? Looks at the tests at org.mobicents.servlet.restcomm.sms for examples how to create test for SMS.

Also the MockSmppServer needs to be modified also in order to support the encoding.

Thanks
George

@alexclear alexclear force-pushed the alexclear:master branch from 6f66d2b to fba3917 Sep 27, 2016

@alexclear

This comment has been minimized.

Contributor

alexclear commented Sep 28, 2016

@gvagenas I added the tests, could you please check them?

Thank you,
Alex

@gvagenas

This comment has been minimized.

Collaborator

gvagenas commented Sep 28, 2016

@alexclear I reviewed again and thanks for adding the test cases.

After your last commit 0c41398 the test case org.mobicents.servlet.restcomm.smpp.SmppTests#testSendMessageToRestcommUCS2 fails, can you please check this.

Please run all the related test cases before you push a commit.

Thanks
George

@alexclear

This comment has been minimized.

Contributor

alexclear commented Sep 28, 2016

@gvagenas Could you please provide logs? I checked the test twice on two different hosts and it worked OK.

Thank you,
Alex

@alexclear

This comment has been minimized.

Contributor

alexclear commented Sep 28, 2016

What I basically did was:

root@vagrant-ubuntu-trusty-64:~/Restcomm-Connect/restcomm/restcomm.testsuite# mvn -Dtest=SmppTests test

and what I got was:

Results :

Tests run: 4, Failures: 0, Errors: 0, Skipped: 0

[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 4:31.949s
[INFO] Finished at: Wed Sep 28 11:39:47 UTC 2016
[INFO] Final Memory: 34M/97M
[INFO] ------------------------------------------------------------------------
@alexclear

This comment has been minimized.

Contributor

alexclear commented Sep 28, 2016

@gvagenas Just tested again in a freshly generated Vagrant environment (I had to tweak build.xml a bit because build 758 seemed to be out of the artifacts repo), the test passed OK.

@gvagenas

This comment has been minimized.

Collaborator

gvagenas commented Sep 28, 2016

@alexclear you are right, something could be wrong on my side because now all tests pass, sorry for that. I will merge now

@gvagenas gvagenas merged commit 0c41398 into RestComm:master Sep 28, 2016

@gvagenas gvagenas removed the Peer Review label Sep 28, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment