Ovirt devel - Sep 2008 - [PATCH server] Fixed the tests for host-browser.

A change to the identify protocol was overlooked. When an empty value was
submitted, host-browser would throw an exception. But, a change allowed
empty values to get by without notice. Fixed.

Also added a BuildRequires dependency on ruby-flexmock so that the RPM is
correctly installed when building the server RPM.

Signed-off-by: Darryl L. Pierce <dpierce at redhat.com>
---
 ovirt-server.spec.in                      |    1 +
 src/host-browser/host-browser.rb          |    9 +++++++--
 src/test/unit/host_browser_awaken_test.rb |    2 +-
 3 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/ovirt-server.spec.in b/ovirt-server.spec.in
index a65e628..f351e77 100644
--- a/ovirt-server.spec.in
+++ b/ovirt-server.spec.in
@@ -35,6 +35,7 @@ Requires(preun): /sbin/chkconfig
 Requires(preun): /sbin/service
 BuildRequires: ruby >= 1.8.1
 BuildRequires: ruby-devel
+BuildRequires: ruby-flexmock
 BuildRequires: ruby-gettext-package
 BuildRequires: rubygem(rake) >= 0.7
 Provides: ovirt-server
diff --git a/src/host-browser/host-browser.rb b/src/host-browser/host-browser.rb
index 3adea03..a33e657 100755
--- a/src/host-browser/host-browser.rb
+++ b/src/host-browser/host-browser.rb
@@ -46,10 +46,15 @@ class HostBrowser
     def initialize(session)
         @session = session
         @keytab_dir = '/usr/share/ipa/html/'
+        set_peeraddr @session.peeraddr[3]
+    end
+    
+    def set_peeraddr(peeraddr)
+      @peeraddr = peeraddr
     end
 
     def prefix(session)
-      "#{Time.now.strftime('%b %d %H:%M:%S')}
#{session.peeraddr[3]} "
+      "#{Time.now.strftime('%b %d %H:%M:%S')} #{@peeraddr} "
     end
 
     # Ensures the conversation starts properly.
@@ -113,7 +118,7 @@ class HostBrowser
                 nic_info << nic
             else
 
-                raise Exception.new("ERRINFO! Expected key=value :
#{info}\n") unless info =~ /[\w]+[\s]*=[\w]*/
+                raise Exception.new("ERRINFO! Expected key=value :
#{info}\n") unless info =~ /[\w]+[\s]*=[\w]+/
 
                 key, value = info.split("=")
 
diff --git a/src/test/unit/host_browser_awaken_test.rb
b/src/test/unit/host_browser_awaken_test.rb
index 5340e01..55a3458 100644
--- a/src/test/unit/host_browser_awaken_test.rb
+++ b/src/test/unit/host_browser_awaken_test.rb
@@ -64,7 +64,7 @@ class HostBrowserAwakenTest < Test::Unit::TestCase
   # Ensures that the server is satisfied if the remote system is
   # making a wakeup call.
   #
-  def test_get_mode_with_awaken_request
+  def test_get_mode_with_awaken_request    
     @session.should_receive(:write).with("MODE?\n").once().returns {
|request| request.length }
     @session.should_receive(:readline).once().returns { "AWAKEN\n" }
 
-- 
1.5.5.1

Hey Darryl,
    I tried applying and running this patch today but ran into some
issues. Comments embedded below.



Darryl L. Pierce wrote:
> A change to the identify protocol was overlooked. When an empty value was
> submitted, host-browser would throw an exception. But, a change allowed
> empty values to get by without notice. Fixed.
>
> Also added a BuildRequires dependency on ruby-flexmock so that the RPM is
> correctly installed when building the server RPM.
>
> Signed-off-by: Darryl L. Pierce <dpierce at redhat.com>
>   
> ---
>  ovirt-server.spec.in                      |    1 +
>  src/host-browser/host-browser.rb          |    9 +++++++--
>  src/test/unit/host_browser_awaken_test.rb |    2 +-
>  3 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/ovirt-server.spec.in b/ovirt-server.spec.in
> index a65e628..f351e77 100644
> --- a/ovirt-server.spec.in
> +++ b/ovirt-server.spec.in
> @@ -35,6 +35,7 @@ Requires(preun): /sbin/chkconfig
>  Requires(preun): /sbin/service
>  BuildRequires: ruby >= 1.8.1
>  BuildRequires: ruby-devel
> +BuildRequires: ruby-flexmock
>  BuildRequires: ruby-gettext-package
>  BuildRequires: rubygem(rake) >= 0.7
>  Provides: ovirt-server
>   
git-am reported some conflict with this file when I applied the patch.
Luckily this file contained a simple one line change so I was able to
manually copy it over and remove the file from the patch.
> diff --git a/src/host-browser/host-browser.rb
b/src/host-browser/host-browser.rb
> index 3adea03..a33e657 100755
> --- a/src/host-browser/host-browser.rb
> +++ b/src/host-browser/host-browser.rb
> @@ -46,10 +46,15 @@ class HostBrowser
>      def initialize(session)
>          @session = session
>          @keytab_dir = '/usr/share/ipa/html/'
> +        set_peeraddr @session.peeraddr[3]
> +    end
> +    
>   
This previous line, though blank, has trailing whitespace.

> +    def set_peeraddr(peeraddr)
> +      @peeraddr = peeraddr
>      end
>  
>      def prefix(session)
> -      "#{Time.now.strftime('%b %d %H:%M:%S')}
#{session.peeraddr[3]} "
> +      "#{Time.now.strftime('%b %d %H:%M:%S')} #{@peeraddr}
"
>      end
>  
>      # Ensures the conversation starts properly.
> @@ -113,7 +118,7 @@ class HostBrowser
>                  nic_info << nic
>              else
>  
> -                raise Exception.new("ERRINFO! Expected key=value :
#{info}\n") unless info =~ /[\w]+[\s]*=[\w]*/
> +                raise Exception.new("ERRINFO! Expected key=value :
#{info}\n") unless info =~ /[\w]+[\s]*=[\w]+/
>  
>                  key, value = info.split("=")
>  
> diff --git a/src/test/unit/host_browser_awaken_test.rb
b/src/test/unit/host_browser_awaken_test.rb
> index 5340e01..55a3458 100644
> --- a/src/test/unit/host_browser_awaken_test.rb
> +++ b/src/test/unit/host_browser_awaken_test.rb
> @@ -64,7 +64,7 @@ class HostBrowserAwakenTest < Test::Unit::TestCase
>    # Ensures that the server is satisfied if the remote system is
>    # making a wakeup call.
>    #
> -  def test_get_mode_with_awaken_request
> +  def test_get_mode_with_awaken_request    
>   
Same issue with trailing whitespace here.

>     
@session.should_receive(:write).with("MODE?\n").once().returns {
|request| request.length }
>      @session.should_receive(:readline).once().returns {
"AWAKEN\n" }
>  
>   

After applying the patch, I tried running the test cases but to no
avail, many were still broken. Doing a bit of debugging myself, I see
that when you recently added tests for the stuff fixed with this patch,
you added a few references to a variable named "@first_id" to the
code.
Looking at this variable I see that it is null everywhere I looked. I'm
not sure if I'm doing something wrong, but I tried googling around for
it but could not find any reference to a preset @first_id provided by
ruby's unit test suite. Adding a "@first_id = 1" to the
"setup" function
of one of the problematic functional tests seemed to mitigate those
errors there (though there were others, not sure the cause). Perhaps you
could provide some insight into this issue.

cc'ing Scott on this email because I also noticed during this process
that some of the tests were still broken due to the recent smart pool
changes. More specifically with the recent commit updating the pool test
fixture, I believe several of the other fixtures, specifically those
with foreign key references to the pools table, are not valid. Grepping
"test/fixtures/" for "pool_id" brings these forward.


In any case, feel free to shout out via email or irc if you want to
figure out / debug this together. Thanks alot!

   -Mo

A change to the identify protocol was overlooked. When an empty value was
submitted, host-browser would throw an exception. But, a change allowed
empty values to get by without notice. Fixed.

Also added a BuildRequires dependency on ruby-flexmock so that the RPM is
correctly installed when building the server RPM.

Signed-off-by: Darryl L. Pierce <dpierce at redhat.com>
---
 ovirt-server.spec.in             |    1 +
 src/host-browser/host-browser.rb |    9 +++++++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/ovirt-server.spec.in b/ovirt-server.spec.in
index 114b52d..5650e01 100644
--- a/ovirt-server.spec.in
+++ b/ovirt-server.spec.in
@@ -36,6 +36,7 @@ Requires(preun): /sbin/service
 BuildRequires: ruby >= 1.8.1
 BuildRequires: ruby-devel
 BuildRequires: rubygem(gettext)
+BuildRequires: ruby-flexmock
 BuildRequires: rubygem(rake) >= 0.7
 Provides: ovirt-server
 BuildArch: noarch
diff --git a/src/host-browser/host-browser.rb b/src/host-browser/host-browser.rb
index 3adea03..2241614 100755
--- a/src/host-browser/host-browser.rb
+++ b/src/host-browser/host-browser.rb
@@ -46,10 +46,15 @@ class HostBrowser
     def initialize(session)
         @session = session
         @keytab_dir = '/usr/share/ipa/html/'
+        set_peeraddr @session.peeraddr[3]
+    end
+
+    def set_peeraddr(peeraddr)
+      @peeraddr = peeraddr
     end
 
     def prefix(session)
-      "#{Time.now.strftime('%b %d %H:%M:%S')}
#{session.peeraddr[3]} "
+      "#{Time.now.strftime('%b %d %H:%M:%S')} #{@peeraddr} "
     end
 
     # Ensures the conversation starts properly.
@@ -113,7 +118,7 @@ class HostBrowser
                 nic_info << nic
             else
 
-                raise Exception.new("ERRINFO! Expected key=value :
#{info}\n") unless info =~ /[\w]+[\s]*=[\w]*/
+                raise Exception.new("ERRINFO! Expected key=value :
#{info}\n") unless info =~ /[\w]+[\s]*=[\w]+/
 
                 key, value = info.split("=")
 
-- 
1.5.5.1

A change to the identify protocol was overlooked. When an empty value was
submitted, host-browser would throw an exception. But, a change allowed
empty values to get by without notice. Fixed.

Also added a Requires dependency on ruby-flexmock so that the RPM is
correctly installed when building the server RPM.

Finally, reintroduced a line that was deleted by mistaken from
NicControllerTest.

Signed-off-by: Darryl L. Pierce <dpierce at redhat.com>
---
 ovirt-server.spec.in                        |    1 +
 src/host-browser/host-browser.rb            |    9 +++++++--
 src/test/functional/nic_controller_test.rb  |    2 ++
 src/test/unit/host_browser_awaken_test.rb   |    3 +++
 src/test/unit/host_browser_identify_test.rb |    3 +++
 5 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/ovirt-server.spec.in b/ovirt-server.spec.in
index 114b52d..1339b52 100644
--- a/ovirt-server.spec.in
+++ b/ovirt-server.spec.in
@@ -19,6 +19,7 @@ Requires: rubygem(mongrel) >= 1.0.1
 Requires: rubygem(krb5-auth) >= 0.6
 Requires: rubygem(cobbler) >= 0.0.2
 Requires: rubygem(gettext)
+Requires: ruby-flexmock
 Requires: postgresql-server
 Requires: ruby-postgres
 Requires: xapian-bindings-ruby
diff --git a/src/host-browser/host-browser.rb b/src/host-browser/host-browser.rb
index 3adea03..2241614 100755
--- a/src/host-browser/host-browser.rb
+++ b/src/host-browser/host-browser.rb
@@ -46,10 +46,15 @@ class HostBrowser
     def initialize(session)
         @session = session
         @keytab_dir = '/usr/share/ipa/html/'
+        set_peeraddr @session.peeraddr[3]
+    end
+
+    def set_peeraddr(peeraddr)
+      @peeraddr = peeraddr
     end
 
     def prefix(session)
-      "#{Time.now.strftime('%b %d %H:%M:%S')}
#{session.peeraddr[3]} "
+      "#{Time.now.strftime('%b %d %H:%M:%S')} #{@peeraddr} "
     end
 
     # Ensures the conversation starts properly.
@@ -113,7 +118,7 @@ class HostBrowser
                 nic_info << nic
             else
 
-                raise Exception.new("ERRINFO! Expected key=value :
#{info}\n") unless info =~ /[\w]+[\s]*=[\w]*/
+                raise Exception.new("ERRINFO! Expected key=value :
#{info}\n") unless info =~ /[\w]+[\s]*=[\w]+/
 
                 key, value = info.split("=")
 
diff --git a/src/test/functional/nic_controller_test.rb
b/src/test/functional/nic_controller_test.rb
index 74a80f4..1fa34ff 100644
--- a/src/test/functional/nic_controller_test.rb
+++ b/src/test/functional/nic_controller_test.rb
@@ -30,6 +30,8 @@ class NicControllerTest < Test::Unit::TestCase
     @controller = NicController.new
     @request    = ActionController::TestRequest.new
     @response   = ActionController::TestResponse.new
+    
+    @first_id = nics(:one).id
   end
 
   def test_show
diff --git a/src/test/unit/host_browser_awaken_test.rb
b/src/test/unit/host_browser_awaken_test.rb
index 5340e01..53711ee 100644
--- a/src/test/unit/host_browser_awaken_test.rb
+++ b/src/test/unit/host_browser_awaken_test.rb
@@ -18,6 +18,9 @@
 # MA  02110-1301, USA.  A copy of the GNU General Public License is
 # also available at http://www.gnu.org/copyleft/gpl.html.
 
+$: << File.join(File.dirname(__FILE__), "../../dutils")
+$: << File.join(File.dirname(__FILE__), "../../host-browser")
+ 
 require File.dirname(__FILE__) + '/../test_helper'
 require 'test/unit'
 require 'flexmock/test_unit'
diff --git a/src/test/unit/host_browser_identify_test.rb
b/src/test/unit/host_browser_identify_test.rb
index 87c6f0b..86f5a73 100644
--- a/src/test/unit/host_browser_identify_test.rb
+++ b/src/test/unit/host_browser_identify_test.rb
@@ -18,6 +18,9 @@
 # MA  02110-1301, USA.  A copy of the GNU General Public License is
 # also available at http://www.gnu.org/copyleft/gpl.html.
 
+$: << File.join(File.dirname(__FILE__), "../../dutils")
+$: << File.join(File.dirname(__FILE__), "../../host-browser")
+ 
 require File.dirname(__FILE__) + '/../test_helper'
 
 require 'test/unit'
-- 
1.5.5.1