Project

General

Profile

Bug #10498

Bug #10288: Fix newly identified issues to make our test suite more robust and faster

SSH tests are fragile

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

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

100%

Feature Branch:
test/10498-ssh-tests-are-fragile
Type of work:
Code
Blueprint:
Starter:
Affected tool:

Description

These could use retrying with a new tor circuit upon failure.

Associated revisions

Revision 78170797 (diff)
Added by kytv about 4 years ago

Retry connecting to SSH servers on failure

When we fail to make a connection via SSH, force a new Tor circuit and
retry.

Will-fix: #10498

Revision f04bd081 (diff)
Added by kytv about 4 years ago

Retry connecting SFTP server on failure

We'll force a new Tor circuit and retry connecting.

Will-fix: #10498

Revision 7c487dbc (diff)
Added by bertagaz over 3 years ago

Fix SFTP scenario.

Needs to confirm we don't care about the SFTP server fingerprint.

Refs: #10498

Revision bc1f00cb (diff)
Added by bertagaz over 3 years ago

Fix merge mess.

Removed the fingerprint verification step by mistake, that's why I had
to add it back in the previous commit. Too bad for the new blob in the
repo.

Refs: #10498

Revision 273dbe2e (diff)
Added by bertagaz over 3 years ago

Wording.

Refs: #10498

Revision 44756480 (diff)
Added by bertagaz over 3 years ago

Kill any ssh process when retrying in the SFTP scenario.

Otherwise it sometimes succeeds to connect after a while, and then the
fingerprint verification window pops up while we're already retrying the
scenario and are at a different step.

Refs: #10498

Revision b1568d61 (diff)
Added by bertagaz over 3 years ago

We don't need to set up sftp_port on each retry.

Refs: #10498

Revision f4e064f1 (diff)
Added by bertagaz over 3 years ago

Close Files before retrying.

Restarting fresh the whole Files application ensure no previous retry
messed up the interface, e.g after the first run, the "connect to
server" button color has changed.

Refs: #10498

Revision 920baa75 (diff)
Added by bertagaz over 3 years ago

Catch more Files error output.

We'll get better output about the reason why the step failed.

Refs: #10498

Revision 36a2d8eb (diff)
Added by bertagaz over 3 years ago

Wait for Files to vanish after closing it.

Refs: #10498

Revision de95a13e (diff)
Added by bertagaz over 3 years ago

Remove racy condition.

Refs: #10498

Revision a2cf03a9 (diff)
Added by bertagaz over 3 years ago

Remove useless SSH error catching and related images.

Refs: #10498

Revision 5fc72cb9
Added by intrigeri over 3 years ago

Merge remote-tracking branch 'origin/test/10498-ssh-tests-are-fragile' into stable

Fix-committed: #10498

History

#1 Updated by kytv about 4 years ago

  • Target version set to Tails_1.8

#2 Updated by kytv about 4 years ago

  • Feature Branch set to kytv/test/10498-ssh-tests-are-fragile

#3 Updated by kytv about 4 years ago

  • Status changed from Confirmed to In Progress
  • % Done changed from 0 to 40

I'm still testing but so far my results are very promising. :)

#4 Updated by kytv about 4 years ago

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

#5 Updated by intrigeri about 4 years ago

  • Assignee changed from anonym to intrigeri

I'll give it a try.

#6 Updated by bertagaz about 4 years ago

  • Assignee changed from intrigeri to bertagaz

intrigeri wrote:

I'll give it a try.

We should really do collective plates rather than stealing each others one! :)

#7 Updated by bertagaz about 4 years ago

  • Target version changed from Tails_1.8 to Tails_2.0

Postponing

#8 Updated by kytv about 4 years ago

  • Assignee changed from bertagaz to kytv

This will need to be updated for Jessie.

#9 Updated by kytv about 4 years ago

  • QA Check changed from Ready for QA to Dev Needed

#10 Updated by intrigeri about 4 years ago

It would be fine to postpone this to 2.2 or something. In any case, please prioritize your SponsorsM4 stuff (Icedove) higher.

#11 Updated by intrigeri about 4 years ago

  • Target version changed from Tails_2.0 to Tails_2.2

#12 Updated by intrigeri almost 4 years ago

  • Target version changed from Tails_2.2 to Tails_2.3

intrigeri wrote:

It would be fine to postpone this to 2.2 or something. In any case, please prioritize your SponsorsM4 stuff (Icedove) higher.

Still the case.

#13 Updated by bertagaz almost 4 years ago

  • Assignee changed from kytv to bertagaz
  • Target version changed from Tails_2.3 to Tails_2.4

#14 Updated by bertagaz over 3 years ago

  • Target version changed from Tails_2.4 to Tails_2.5

#15 Updated by bertagaz over 3 years ago

  • Assignee changed from bertagaz to anonym
  • % Done changed from 50 to 70
  • QA Check changed from Dev Needed to Ready for QA
  • Feature Branch changed from kytv/test/10498-ssh-tests-are-fragile to test/10498-ssh-tests-are-fragile

SSH tests had their retry_tor wrapping done without much troubles, but I've finally been able to implement something robust for the SFTP scenario, which was the most problematic. At least my testing at home says so, so it's ready for a review.

#16 Updated by intrigeri over 3 years ago

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

What's the value of checking for a (limited list) of error conditions like SSHFailed.png, SSHError.png and SSHDenied.png? (I'm asking the person who is proposing this branch for merging, not the one who wrote that code initially.)

Same question for 920baa7. You wrote "We'll get better output about the reason why the step failed", but then how is that output useful? It seems to me that it adds basically nothing compared to looking at the failure screenshot, while it gives us more code and images to maintain.

The wording change brought by 273dbe2 makes the text less correct: a SFTP server has no fingerprint; it has a key, that itself has a fingerprint. I advice simply reverting to the previous wording, that feels more correct technically, and was written with someone who's better at English than you or I.

Here also, please drop the useless and racy if $vm.has_process? test when killing processes.

I have concerns about f4e064f: clicking the close button doesn't guarantee that Files has been closed successfully on next try, so I don't think that this change reliably fixes the problem it tries to address. Of course I totally trust that it works for you, and this change likely won't make things any worse, but once it's deployed on dozens of branches, we might realize that you were right in identifying a problem, but that the fix is insufficient. What do you think?

Great work!

#17 Updated by bertagaz over 3 years ago

  • Assignee changed from bertagaz to intrigeri
  • QA Check changed from Dev Needed to Ready for QA

intrigeri wrote:

What's the value of checking for a (limited list) of error conditions like SSHFailed.png, SSHError.png and SSHDenied.png? (I'm asking the person who is proposing this branch for merging, not the one who wrote that code initially.)

Same question for 920baa7. You wrote "We'll get better output about the reason why the step failed", but then how is that output useful? It seems to me that it adds basically nothing compared to looking at the failure screenshot, while it gives us more code and images to maintain.

You make a point. Removed them with 4e5e1fa and a2cf03a

The wording change brought by 273dbe2 makes the text less correct: a SFTP server has no fingerprint; it has a key, that itself has a fingerprint. I advice simply reverting to the previous wording, that feels more correct technically, and was written with someone who's better at English than you or I.

Ok, 9a9ad14

Here also, please drop the useless and racy if $vm.has_process? test when killing processes.

de95a13

I have concerns about f4e064f: clicking the close button doesn't guarantee that Files has been closed successfully on next try, so I don't think that this change reliably fixes the problem it tries to address. Of course I totally trust that it works for you, and this change likely won't make things any worse, but once it's deployed on dozens of branches, we might realize that you were right in identifying a problem, but that the fix is insufficient. What do you think?

Good point. Would 36a2d8e satisfy you?

#18 Updated by intrigeri over 3 years ago

  • Assignee changed from intrigeri to anonym

I have concerns about f4e064f: clicking the close button doesn't guarantee that Files has been closed successfully on next try, so I don't think that this change reliably fixes the problem it tries to address. Of course I totally trust that it works for you, and this change likely won't make things any worse, but once it's deployed on dozens of branches, we might realize that you were right in identifying a problem, but that the fix is insufficient. What do you think?

Good point. Would 36a2d8e satisfy you?

I guess waiting for something more specific to Files to disappear would be more robust / future-proof, but this seems good enough to me => code review passes for me => assigning to anonym for the last review'n'merge steps :)

#19 Updated by intrigeri over 3 years ago

  • Assignee changed from anonym to intrigeri

#20 Updated by intrigeri over 3 years ago

  • Status changed from In Progress to 11
  • Assignee deleted (intrigeri)
  • % Done changed from 70 to 100
  • QA Check changed from Ready for QA to Pass

Merged, thanks!

#21 Updated by intrigeri over 3 years ago

  • Status changed from 11 to Resolved

Also available in: Atom PDF