Project

General

Profile

Bug #8742

Feature #5330: Test suite: identify and document race conditions

Synaptic tests frequently fail

Added by kytv over 4 years ago. Updated over 4 years ago.

Status:
Resolved
Priority:
Normal
Assignee:
-
Category:
Test suite
Target version:
Start date:
01/19/2015
Due date:
% Done:

100%

Feature Branch:
kytv:bugfix/8742-synaptic-test-robustness
Type of work:
Code
Blueprint:
Starter:
Affected tool:

Description

I'm working on resolving this by changing the hardcoded sleeps into @screen.waits. The result of this work makes this feature much more reliable during my test runs.

The current remaining frequent failure ("Tor is ready") will be fixed once anonym's fix for #8714 is merged.

Associated revisions

Revision 80b215fb
Added by Tails developers over 4 years ago

Merge remote-tracking branch 'kytv/bugfix/8742-synaptic-test-robustness' into testing

Fix-committed: #8742

History

#1 Updated by intrigeri over 4 years ago

The current remaining frequent failure ("Tor is ready") will be fixed once anonym's fix for #8714 is merged.

It's been merged 9 hours ago :)

#2 Updated by kytv over 4 years ago

intrigeri wrote:

The current remaining frequent failure ("Tor is ready") will be fixed once anonym's fix for #8714 is merged.

It's been merged 9 hours ago :)

Well then, a correction is in order: :)

The upcoming feature branch for this ticket will fix the problems that I experienced in the apt feature. (I just want to run it a dozen more times (or so) before pushing).

#3 Updated by kytv over 4 years ago

  • Assignee changed from kytv to anonym
  • % Done changed from 20 to 40
  • QA Check set to Ready for QA
  • Feature Branch set to kytv:bugfix/8742-synaptic-test-robustness

#4 Updated by intrigeri over 4 years ago

  • Assignee changed from anonym to kytv
  • QA Check changed from Ready for QA to Dev Needed

Your branch bundles a commit from kytv:test/8697-update-apt-get, but its history was rewritten so it has a different ID. Please either rebase on top of devel, or rebase on top of kytv:test/8697-update-apt-get and mark this ticket as blocked by #8697. Thanks!

#5 Updated by kytv over 4 years ago

  • QA Check changed from Dev Needed to Ready for QA

Oops. Rebased on devel. Sorry about that.

#6 Updated by kytv over 4 years ago

  • Assignee changed from kytv to anonym

#7 Updated by anonym over 4 years ago

  • Assignee changed from anonym to kytv
  • QA Check changed from Ready for QA to Dev Needed

kytv wrote:

Oops. Rebased on devel. Sorry about that.

Hmm, after fetching your repo git log --oneline origin/devel..kytv/bugfix/8742-synaptic-test-robustness still lists:

3618e8e Ensure that the test will fail if "apt-get X" commands fail.

which seems to be an other version of de8959c, which now is in devel after I merged kytv:test/8697-update-apt-get. Please drop 3618e8e and force push.

Here's a review of what's currently commit d6ff7d0 ("Reduce the number of static sleeps."):

+ @screen.wait('SynapticPackageList.png', 30)

You forgot to git add features/images/SynapticPackageList.png. :)

# We do this after a Reload, so the interface will be frozen until
# the package list has been loaded
- try_for(60, :msg => "Failed to open the Synaptic 'Find' window") {
- @screen.type("f", Sikuli::KeyModifier.CTRL) # Find key
- @screen.find('SynapticSearch.png')
- }
+ @screen.type("f", Sikuli::KeyModifier.CTRL) # Find key
+ @screen.wait_and_click('SynapticSearch.png', 10)

That try_for block has proven essential before since the GUI is unresponsive, but I assume that the point of waiting for SynapticPackageList.png in the previous step is to ensure that the GUI is responsive. If so, great! But please remove the comment, and possibly add an appropriate comment above screen.wait('SynapticPackageList.png', 30).

+ @screen.wait('SynapticCowsaySearchResultSelected.png', 20)
[...]
+ @screen.wait('SynapticCowsayMarked.png', 10)
+ @screen.wait_and_click('SynapticApply.png', 10)

You forgot to @git add" these images too. :)

- @screen.type("p", Sikuli::KeyModifier.CTRL) # Apply
[...]
+ ("p", Sikuli::KeyModifier.CTRL) # Apply

Don't comment code; kill it! :)

+ @screen.wait_and_click('SynapticPolicyKitAuthPrompt.png', 30)
deal_with_polkit_prompt('SynapticPolicyKitAuthPrompt.png', @sudo_password)

If the _click indeed is necessary I recommend moving the wait_and_click into the deal_with_polkit_prompt helper (located in features/step_definitions/common_steps.rb). Here's how it looks:

def deal_with_polkit_prompt (image, password)
@screen.wait(image, 60)
[...]

So replace that wait with wait_and_click.

I'd also like to raise an interesting issue here: in the current state of the code, the wait_and_click you added would move the mouse cursor on top of the image we later wait for on in deal_with_polkit_prompt, hence potentially blocking it from being found by Sikuli. Sure, we use unclutter to try to hide the cursor, but it is pretty unreliable, and we should really try to explicitly avoid situations like this when possible. Yes, Tails' automated test suite is an interesting place... :)

However, it may be that the _click isn't needed. I have a theory; let's look a bit more on deal_with_polkit_prompt:

def deal_with_polkit_prompt (image, password)
@screen.wait(image, 60)
sleep 1 # wait for weird fade-in to unblock the "Ok" button
@screen.type(password)
[...]

Perhaps what you really is hit by is that the sleep 1 isn't enough for you? Actually it's unclear to me what your exact issue with this test is, but I suggest you experiment a bit more with the hypothesis that it's this static sleep that's the culprit. Perhaps the situation can be improved by wait:ing on the OK button to unfreeze, or similar?

#9 Updated by intrigeri over 4 years ago

Ping?

#10 Updated by kytv over 4 years ago

Back on this and my other assigned tickets.

#11 Updated by kytv over 4 years ago

  • Assignee changed from kytv to anonym
  • % Done changed from 40 to 50
  • QA Check changed from Dev Needed to Ready for QA

Rebased on devel and force pushed.

#12 Updated by kytv over 4 years ago

Previously the tests with Synaptic failed frequently. With the changes in this branch, the only failures arose when they should have.

#13 Updated by anonym over 4 years ago

  • Assignee changed from anonym to kytv
  • QA Check changed from Ready for QA to Info Needed

+ #@screen.wait_and_click('SynapticPolicyKitAuthPrompt.png', 30)

Again, let's not add any commented out code.

- sleep 1 # wait for weird fade-in to unblock the "Ok" button
+ sleep 3 # wait for weird fade-in to unblock the "Ok" button

I'm surprised this is needed. I actually had a closer look, and the "weird fade-in" isn't even happening in Wheezy, but was something Squeeze-specific. I'd expect us to be able to remove the sleep, but obviously you experience some sort of issue here. Can you please describe what the issue is more precisely?

#14 Updated by kytv over 4 years ago

  • Target version changed from Tails_1.3 to Tails_1.3.2

#15 Updated by kytv over 4 years ago

  • Target version changed from Tails_1.3.2 to Tails_1.3

#16 Updated by kytv over 4 years ago

  • Assignee changed from kytv to anonym
  • QA Check changed from Info Needed to Ready for QA

anonym wrote:

I'm surprised this is needed.

You were absolutely right. I removed the sleep and successfully re-ran this test multiple times without a hiccup. I don't recall what I had seen before, but I'm certainly not seeing it now.

I merged in the testing branch, removed the sleep statement and the commented out line which was inadvertently left in.

#17 Updated by Tails over 4 years ago

  • Status changed from In Progress to Fix committed
  • % Done changed from 50 to 100

Applied in changeset commit:c9aeac15bd4da41ce79fc7c073fd0ff22cd3940e.

#18 Updated by anonym over 4 years ago

  • Assignee deleted (anonym)
  • QA Check changed from Ready for QA to Pass

Merged!

#19 Updated by BitingBird over 4 years ago

  • Status changed from Fix committed to Resolved

Also available in: Atom PDF