stompgem / stomp

A ruby gem for sending and receiving messages from a Stomp protocol compliant message queue. Includes: failover logic, ssl support.
http://stomp.github.com
Apache License 2.0
152 stars 80 forks source link

Client headers - given hash should not be changed inside Client methods #126

Closed mikola-spb closed 8 years ago

mikola-spb commented 8 years ago

PR for #125 I covered all client methods with headers has as parameter. So specs cover not only hash immutability but also if headers passed to a connection. There is also Client#find_listener() method, which modifies message headers, but it's out of this scope. And I'm not sure if it should be changed.

gmallard commented 8 years ago

Thanks for spending time on this.

At the HEAD of your client_headers branch I can run the spec tests without problems. So that is good.

However the real unit tests fail.

So please give this PR another go.

The default unit tests need a running broker on your machine (this can be changed), and expect that:

Run unit tests by:

rake test

Currently, the HEAD of your client_headers branch gives me:

[gmallard@tjjackson stompgem (mokola-spb-ch)]$ rake test
/opt/r224_232/bin/ruby2.2.4 -I"lib:test"  "/opt/r224_232/lib/ruby/2.2.0/rake/rake_test_loader.rb" "test/test_anonymous.rb" "test/test_client.rb" "test/test_codec.rb" "test/test_connection.rb" "test/test_connection1p.rb" "test/test_helper.rb" "test/test_message.rb" "test/test_ssl.rb" "test/test_urlogin.rb" 
Loaded suite /opt/r224_232/lib/ruby/2.2.0/rake/rake_test_loader
Started
............................................F
================================================================================
Failure: test_raise_on_multiple_subscriptions_to_same_id_mixed(TestClient)
/ad3/gma/stomp-repos/stompgem/test/test_client.rb:316:in `test_raise_on_multiple_subscriptions_to_same_id_mixed'
     313:   def test_raise_on_multiple_subscriptions_to_same_id_mixed
     314:     subscribe_dest = make_destination
     315:     @client.subscribe(subscribe_dest, {'id' => 'myid'}) {|m| nil }
  => 316:     assert_raise(RuntimeError) do
     317:       @client.subscribe(subscribe_dest, {:id => 'myid'}) {|m| nil }
     318:     end
     319:     checkEmsg(@client)

<RuntimeError> expected but was
<Stomp::Error::DuplicateSubscription(<duplicate subscriptions are disallowed>)>

diff:
? Runtime  Error                                                                 
? S   o p::     ::DuplicateSubscription(<duplicate subscriptions are disallowed>)
================================================================================
........................................................

Finished in 16.5173267 seconds.
--------------------------------------------------------------------------------
101 tests, 470 assertions, 1 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
99.0099% passed
--------------------------------------------------------------------------------
6.11 tests/s, 28.45 assertions/s
rake aborted!
Command failed with status (1): [ruby -I"lib:test"  "/opt/r224_232/lib/ruby/2.2.0/rake/rake_test_loader.rb" "test/test_anonymous.rb" "test/test_client.rb" "test/test_codec.rb" "test/test_connection.rb" "test/test_connection1p.rb" "test/test_helper.rb" "test/test_message.rb" "test/test_ssl.rb" "test/test_urlogin.rb" ]

Tasks: TOP => test
(See full trace by running task with --trace)

When running unit tests you can change the broker and port. An example:

STOMP_HOST=otherhost STOMP_PORT=12345 rake test

The host can be an IP address if needed.

The unit tests should run on at least one broker.

FYI, this is how I run unit tests locally:

stomp gem unit tests

If you are running this on a Windows box and need help with that please let me know. The Windows unit tests .cmd file is different of course.

mikola-spb commented 8 years ago

Thanks for reviewing! I was trying to run tests without my changes against Apache Artemis 1.2/1.3 without success. I use MacBook. I see you use different brokers for testing. That would be nice to see somewhere how broker should be configured. AMQ with default configuration works fine (I can fix that stupid copy-paste bug). I mean this stomp should work with Artemis as well, but seems that id has to be configured somehow.

gmallard commented 8 years ago

I will also review this tomorrow.

Thanks a lot for your efforts here.

On 07/14/2016 10:14 AM, Nikolay Khasanov wrote:

Thanks for reviewing! I was trying to run tests without my changes against Apache Artemis 1.2/1.3 without success. I use MacBook. I see you use different brokers for testing. That would be nice to see somewhere how broker should be configured. AMQ with default configuration works fine (I can fix that stupid copy-paste bug). I mean this stomp should work with Artemis as well, but seems that id has to be configured somehow.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/stompgem/stomp/pull/126#issuecomment-232677674, or mute the thread https://github.com/notifications/unsubscribe/AAAbv0s6JKj31rPi_zpL7LOETrxVO8klks5qVkQ4gaJpZM4JLYf5.