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

[Mohammed Morsi]

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