[Ovirt-devel] [PATCH] Fixes to broken tests (1/2)

[Mohammed Morsi]

Alan Pevec wrote:
>> @@ -114,12 +114,14 @@ if File.exists? File.dirname(__FILE__) +
>> '/../selenium.rb'
>>              
>> "selenium.isElementPresent(\"//div[@id='vm_action_results']/div[3]/div/div[2]/a\")",
>>
>>                10000)
>>          @browser.click
>> "//div[@id='vm_action_results']/div[3]/div/div[2]/a"
>> +        sleep 5 # give vm time to start
>
> Why is sleep needed here at all if wait_for_condition below is
> expecting 'running' to show up?
> In general, using sleep for synchronization is wrong, although it
> might seem to work.
As far as I can tell the vm tab doesn't get automatically updated via
javascript when the state of a vm changes, thus I need to wait a little
time and manually refresh the page before I check for the status. I
could do an assertion on the backend, and check the actual vm table
entry itself, but these are interface tests, thus assertions should be
on interface components.


>
>>          @browser.click "//div[@id='side']/ul/li/ul/li[1]/span/a"
>>          @browser.wait_for_condition(
>> -            
>> "selenium.isElementPresent(\"//table[@id='vms_grid']/tbody/tr[" +
>> (num_vms.to_i + 1).to_s + "]/td[7]/div\")",
>> +            
>> "selenium.isElementPresent(\"//table[@id='vms_grid']/tbody/tr[" +
>> (num_vms.to_i + 1).to_s + "]/td[7]/div\") && " +
>> +             "selenium.getText(\"//table[@id='vms_grid']/tbody/tr["
>> + (num_vms.to_i + 1).to_s + "]/td[7]/div\") == \"running\"",
>>                20000)
>> -        assert_equal("running",
>> -          @browser.get_text("//table[@id='vms_grid']/tbody/tr[" +
>> (num_vms.to_i + 1).to_s + "]/td[7]/div"))
>> +        #assert_equal("running",
>> +          #@browser.get_text("//table[@id='vms_grid']/tbody/tr[" +
>> (num_vms.to_i + 1).to_s + "]/td[7]/div"))
>
> What happens if wait_for_condition times out? Will selenium raise an
> exception, failing the test or it just continues? If latter, you need
> to keep the assert.
> In general, if removing lines, just delete them instead of commenting,
> we have git to keep the history.
Selenium raises an exception when wait_for_condition times out and the
test fails so the assertion can be safely removed. My bad on the
commenting, just code I forgot to remove, will keep it in mind in the
future.


>  
>>          # stop / destroy vm
>>          @browser.click "//table[@id='vms_grid']/tbody/tr[" +
>> (num_vms.to_i + 1).to_s + "]/td[1]/div/input"
>> @@ -128,12 +130,14 @@ if File.exists? File.dirname(__FILE__) +
>> '/../selenium.rb'
>>              
>> "selenium.isElementPresent(\"//div[@id='vm_action_results']/div[3]/div/div[2]/a\")",
>>
>>                10000)
>>          @browser.click
>> "//div[@id='vm_action_results']/div[3]/div/div[2]/a"
>> +        sleep 5 # give vm time to stop
>
> same questions as above
>
>>          @browser.click "//div[@id='side']/ul/li/ul/li[1]/span/a"
>>          @browser.wait_for_condition(
>> -            
>> "selenium.isElementPresent(\"//table[@id='vms_grid']/tbody/tr[" +
>> (num_vms.to_i + 1).to_s + "]/td[7]/div\")",
>> +            
>> "selenium.isElementPresent(\"//table[@id='vms_grid']/tbody/tr[" +
>> (num_vms.to_i + 1).to_s + "]/td[7]/div\") && " +
>> +             "selenium.getText(\"//table[@id='vms_grid']/tbody/tr["
>> + (num_vms.to_i + 1).to_s + "]/td[7]/div\") == \"stopped\"",
>>                20000)
>> -        assert_equal("stopped",
>> -          @browser.get_text("//table[@id='vms_grid']/tbody/tr[" +
>> (num_vms.to_i + 1).to_s + "]/td[7]/div"))
>> +        #assert_equal("stopped",
>> +          #@browser.get_text("//table[@id='vms_grid']/tbody/tr[" +
>> (num_vms.to_i + 1).to_s + "]/td[7]/div"))
>