Page MenuHomePhabricator

Remove "sleep" from tests
Closed, DeclinedPublic

Description

We are using sleep in several places. Sleep is evil. The code should be refactored so the sleep is removed.

There are more robust ways to wait for something to happen than using sleep:

http://watirwebdriver.com/waiting/
https://github.com/cheezy/page-object/wiki/Ajax-Calls

$ grep --include *.rb  -R -F 'sleep' .

./wmde/WikidataBrowserTests/tests/browser/features/support/env.rb:  sleep env_no * 4 # sleep time to give webdriver time to setup
./wmde/WikidataBrowserTests/tests/browser/features/support/modules/entity_module.rb:    sleep(1.0 / 3) while execute_script('return jQuery.active') != 0
./wmde/WikidataBrowserTests/tests/browser/features/support/modules/entity_module.rb:    sleep 1

./mediawiki/selenium/features/step_definitions/environment_steps.rb:    sleep 0.3

./mediawiki/vagrant/mediawiki/extensions/CirrusSearch/tests/browser/features/step_definitions/general_steps.rb:  sleep(Integer(seconds))
./mediawiki/vagrant/mediawiki/extensions/CirrusSearch/tests/browser/features/step_definitions/search_steps.rb:    sleep(5)
./mediawiki/vagrant/mediawiki/extensions/CirrusSearch/tests/browser/features/step_definitions/search_steps.rb:    sleep 1

./mediawiki/vagrant/mediawiki/extensions/Gather/tests/browser/features/step_definitions/common_steps.rb:  sleep 5
./mediawiki/vagrant/mediawiki/extensions/Gather/tests/browser/features/step_definitions/edit_collection_steps.rb:  sleep 5

./mediawiki/vagrant/mediawiki/extensions/MobileFrontend/tests/browser/features/step_definitions/editor_ve_steps.rb:    sleep 2 # this gets around a race condition bug in ChromeDriver where both the confirm and the toast are in the page at once, and Chrome reports either "stale element reference: element is not attached to the page document" or "Element does not exist in cache"
./mediawiki/vagrant/mediawiki/extensions/MobileFrontend/tests/browser/features/step_definitions/notification_steps.rb:  sleep 1
./mediawiki/vagrant/mediawiki/extensions/MobileFrontend/tests/browser/features/step_definitions/notification_steps.rb:  sleep seconds.to_i

./mediawiki/vagrant/mediawiki/extensions/MultimediaViewer/tests/browser/features/step_definitions/mmv_download_steps.rb:  sleep 1

./mediawiki/vagrant/mediawiki/extensions/UploadWizard/tests/browser/features/step_definitions/upload_wizard_steps.rb:    sleep 1 # Sleep because of annoying JS animation happening in this menu
./mediawiki/vagrant/mediawiki/extensions/UploadWizard/tests/browser/features/step_definitions/upload_wizard_steps.rb:    sleep 1 # Sleep because of annoying JS animation happening in this menu
./mediawiki/vagrant/mediawiki/extensions/UploadWizard/tests/browser/features/step_definitions/upload_wizard_steps.rb:    sleep 1 # Sleep because of annoying JS animation happening in this menu
./mediawiki/vagrant/mediawiki/extensions/UploadWizard/tests/browser/features/step_definitions/upload_wizard_steps.rb:    sleep 1 # Sleep because of annoying JS animation happening in this menu
./mediawiki/vagrant/mediawiki/extensions/UploadWizard/tests/browser/features/step_definitions/upload_wizard_steps.rb:  sleep 0.5 # Sleep because of annoying JS animation happening in the date picker

./mediawiki/vagrant/mediawiki/extensions/VisualEditor/modules/ve-mw/tests/browser/features/step_definitions/cite_steps.rb:      sleep 1
./mediawiki/vagrant/mediawiki/extensions/VisualEditor/modules/ve-mw/tests/browser/features/step_definitions/cite_steps.rb:      sleep 1
./mediawiki/vagrant/mediawiki/extensions/VisualEditor/modules/ve-mw/tests/browser/features/step_definitions/cite_steps.rb:      sleep 1
./mediawiki/vagrant/mediawiki/extensions/VisualEditor/modules/ve-mw/tests/browser/features/step_definitions/cite_steps.rb:      sleep 1
./mediawiki/vagrant/mediawiki/extensions/VisualEditor/modules/ve-mw/tests/browser/features/step_definitions/media_interface_steps.rb:    sleep 1

Details

Reference
bz46887

Event Timeline

bzimport raised the priority of this task from to Low.Nov 22 2014, 1:42 AM
bzimport set Reference to bz46887.
bzimport added a subscriber: Unknown Object (MLST).
This comment was removed by zeljkofilipin.
This comment was removed by zeljkofilipin.
This comment was removed by zeljkofilipin.
This comment was removed by zeljkofilipin.

(In reply to comment #3)

Sleep statements found:

Removing the code from the following places will do ?

Tony Thomas: just removing the code is not enough. The tests have to be made to wait properly for whatever it is they are waiting for.

Using sleep() is either an interim step that never got completed, or else the person writing the test could not think of a better way to wait for whatever the test needs.

So the tests need to be updated in a thoughtful way so that they wait correctly without using sleep() but also without failing mistakenly.

This comment was removed by zeljkofilipin.

Tony and I have paired on getting his environment set up to work on this. Tony, if you still are interested in working on this, feel free to assign the bug to yourself. Let me know if you need help.

(In reply to comment #8)

Tony and I have paired on getting his environment set up to work on this.

I will put my status in [QA] once I have made my environment ready.

Change 108916 had a related patch set uploaded by Zfilipin:
Replaced sleep with page-object gem waiting API

https://gerrit.wikimedia.org/r/108916

Change 108918 had a related patch set uploaded by 01tonythomas:
Removed sleep with waiting API in browsertests

https://gerrit.wikimedia.org/r/108918

Change 108918 merged by jenkins-bot:
Removed sleep with waiting API in browsertests

https://gerrit.wikimedia.org/r/108918

Change 108916 merged by Cmcmahon:
Replaced sleep with page-object gem waiting API

https://gerrit.wikimedia.org/r/108916

Change 109471 had a related patch set uploaded by 01tonythomas:
Removed sleep variable from TwnMainPage tests

https://gerrit.wikimedia.org/r/109471

Cirrus has two sleeps:

  1. It waits for some suggestions not to appear. Without reworking the error handling in the javascript I can't remove this. I don't feel up to reworking that right now.
  2. It sleeps between the creation of two pages that it uses to test searches that prefer pages with recent edits. If I could backdate edit times then I wouldn't need it but I can't at this point.

In total Cirrus sleeps 35 seconds because each of the above is executed once. It could be better but it could be worse.

Change 109471 merged by jenkins-bot:
Removed sleep variable from TwnMainPage tests

https://gerrit.wikimedia.org/r/109471

All patches merged => resetting status from PATCH_TO_REVIEW to NEW

Are there more tests containing sleep or is this fixed?

Tony: Are you still working on this or should the assignee be reset to default?

(In reply to Andre Klapper from comment #18)

Are there more tests containing sleep or is this fixed?
Tony: Are you still working on this or should the assignee be reset to
default?

I think there are a few more sleep keywords to be removed. Sorry, but I dont think I will be able to continue on this right now. Resetting to default assignee for the time being.

We can keep this open. I actually just added a sleep to a test because I needed to get on with other things, and I should return to fix it at some point.

demon removed a subscriber: demon.Dec 16 2014, 6:10 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 30 2015, 3:27 PM
zeljkofilipin updated the task description. (Show Details)
zeljkofilipin removed zeljkofilipin as the assignee of this task.Oct 1 2015, 12:27 PM
zeljkofilipin updated the task description. (Show Details)
zeljkofilipin moved this task from In Progress to Ruby on the Browser-Tests-Infrastructure board.
Krinkle removed a subscriber: Krinkle.Mar 8 2017, 4:32 AM
zeljkofilipin closed this task as Declined.May 26 2017, 3:34 PM

Unlikely to ever be resolved because of T139740: Port Selenium tests from Ruby to Node.js.