sstephenson / hector

A private group chat server for people you trust
MIT License
131 stars 15 forks source link

Test failure; undefined method "expects" #38

Closed daBrado closed 10 years ago

daBrado commented 10 years ago

When running the tests via rake, I got an error about expects, e.g.:

[ 54/125] Hector::ConnectionTest#test :"disconnecting should destroy the session" = 0.00 s
  1) Error:
Hector::ConnectionTest#test :"disconnecting should destroy the session":
NoMethodError: undefined method `expects' for #<Hector::UserSession:0x00000002058320>
...

I think something must have changed with the most recent Mocha, because its site says one should require mocha/unit_test, i.e.:

diff --git a/test/test_helper.rb b/test/test_helper.rb
index 6d0c734..809ffa4 100644
--- a/test/test_helper.rb
+++ b/test/test_helper.rb
@@ -1,7 +1,7 @@
 begin
   require "hector"
   require "test/unit"
-  require "mocha"
+  require "mocha/test_unit"
 rescue LoadError => e
   if require "rubygems"
     retry

...doing that makes the tests pass fine. Should that change be included? Perhaps also a version bump for Mocha in the gemspec...

Thanks,

raws commented 10 years ago

That does indeed appear to be the case! If you bundle these changes up in a pull request, I'd be happy to accept it :)

daBrado commented 10 years ago

There you go!

When I ran gem build, it gave some warning about a lack of license, and use of semantic versioning for dependencies... would you like a change to get rid of those, too?

raws commented 10 years ago

Sure! You should be able to use the MIT license abbreviation in the gemspec. For the gem dependencies, I think the following version constraints should work:

s.add_dependency "eventmachine", "~> 0.12.10"
s.add_development_dependency "mocha", "~> 1.0.0"

If you add those changes to your existing pull request, that'd be great. Thanks for taking care of this!

daBrado commented 10 years ago

You are welcome!

I did the event machine dependency a little differently, instead following the example given by the gem's warning, since ~> 0.12.10 I think would imply no version equal to or greater than 0.13.

raws commented 10 years ago

Yeah, I haven't tested Hector with EventMachine >= 1.0.0. Though if you have and everything works okay, we can certainly update the EventMachine dependency!

daBrado commented 10 years ago

Indeed, it seems to be:

% gem list eventmachine
*** LOCAL GEMS ***
eventmachine (1.0.3)
% rake
/usr/bin/ruby -I"lib:test" -I"/usr/lib/ruby/2.1.0" "/usr/lib/ruby/2.1.0/rake/rake_test_loader.rb" "test/integration/async_authentication_test.rb" "test/integration/channels_test.rb" "test/integration/connection_test.rb" "test/integration/keep_alive_test.rb" "test/integration/messaging_test.rb" "test/integration/services_test.rb" "test/unit/identity_test.rb" "test/unit/irc_error_test.rb" "test/unit/request_test.rb" "test/unit/response_test.rb" "test/unit/session_test.rb" "test/unit/yaml_identity_adapter_test.rb" 
Run options: 
# Running tests:
Finished tests in 3.479675s, 35.9229 tests/s, 91.6752 assertions/s.                                                                                               
125 tests, 319 assertions, 0 failures, 0 errors, 0 skips
ruby -v: ruby 2.1.1p76 (2014-02-24 revision 45161) [x86_64-linux]
raws commented 10 years ago

Sounds good to me. Go for it!

daBrado commented 10 years ago

Cool. I think it is part of the pull request now.

daBrado commented 10 years ago

Oh, oops, now it is part of the pull request...

raws commented 10 years ago

Great! Did you test using EventMachine 1.0.0, or 1.0.3, which is the latest release? If 1.0.3, the only change I'd suggest is using s.add_runtime_dependency "~> 1.0.3".

daBrado commented 10 years ago

Oh, indeed, I tested using 1.0.3... I updated the dependency. Instead I did "~> 1.0", ">= 1.0.3", so that it isn't locked into 1.0.3.x, which I think is what would happen with ~> 1.0.3. I also just changed the previous commit, versus doing a new commit... hope that is okay.

raws commented 10 years ago

Ah, you're right, good call. Everything looks great, so I merged your PR. Thank you for contributing!

daBrado commented 10 years ago

Thanks; I'm glad I could. :)