Feature #14817

Test suite improvements from buster sprint 1

Added by anonym 2017-10-09 12:17:58 . Updated 2018-01-09 20:55:27 .

Status:
Resolved
Priority:
Elevated
Assignee:
Category:
Test suite
Target version:
Start date:
2017-10-22
Due date:
% Done:

100%

Feature Branch:
test/improvements-from-buster-sprint
Type of work:
Code
Blueprint:

Starter:
Affected tool:
Deliverable for:

Description

This includes the fabled --fuzzy-image-matching option for the automated test suite, among a few other minor fixes.

Please review’n’merge into devel.


Subtasks


Related issues

Blocks Tails - Feature #13240: Core work 2017Q4: Test suite maintenance Resolved 2017-06-29
Blocked by Tails - Bug #15007: "Pidgin's D-Bus interface is not available" test suite step fails Resolved 2017-11-29
Blocked by Tails - Bug #14929: 'I can view a file stored in "~/Tor Browser" but not in ~/.gnupg' scenario is broken Resolved 2017-11-07
Blocks Tails - Bug #14874: "Failing to add a certificate to Pidgin" test scenario always fails on devel since Sikuli fuzzy matching was implemented Resolved 2017-10-22

History

#1 Updated by intrigeri 2017-10-09 12:32:22

  • Target version changed from Tails_3.5 to Tails_3.6

#2 Updated by intrigeri 2017-10-09 13:03:46

  • Status changed from Confirmed to Fix committed
  • % Done changed from 50 to 100

Applied in changeset commit:7c92cec5853d88c2b96d6e93614a21cd35e8c700.

#3 Updated by intrigeri 2017-10-09 13:34:39

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

Merged into devel & feature/buster. thanks!

#4 Updated by intrigeri 2017-10-10 05:11:52

  • Status changed from Fix committed to In Progress
  • Assignee set to anonym
  • Priority changed from Normal to Elevated
  • % Done changed from 100 to 70
  • QA Check changed from Pass to Dev Needed

Two issues noticed after merging:

  • This seems to break maybe_deal_with_pinentry when the passphrase is cached, see Feature #14814#note-2. I suspect that’ll break stuff on the devel branch too so raising priority. We can’t see it on Jenkins as the affected tests are flagged as fragile.
  • The candidate pics are not saved as artifacts on Jenkins => please file a ticket about it, assigned to me.

#5 Updated by intrigeri 2017-10-10 05:12:16

  • blocks Feature #13240: Core work 2017Q4: Test suite maintenance added

#6 Updated by intrigeri 2017-10-10 05:12:30

  • Target version changed from Tails_3.6 to Tails_3.3

#7 Updated by anonym 2017-10-11 16:19:21

  • Assignee changed from anonym to intrigeri
  • % Done changed from 70 to 80
  • QA Check changed from Dev Needed to Ready for QA

intrigeri wrote:
> Two issues noticed after merging:
>
> * This seems to break maybe_deal_with_pinentry when the passphrase is cached, see Feature #14814#note-2. I suspect that’ll break stuff on the devel branch too so raising priority. We can’t see it on Jenkins as the affected tests are flagged as fragile.

Your suspicious from Feature #14814 was not the case; the nature of the error turned out to be much more arcane, about how Java exceptions are integrated into Ruby via rjb. Fixed in commit:f2744487a2e0d4d52554c516e6f1d25d3f3c32e3.

While investigating your suspicion, I realized findAny() needed some love too (now that some code runs unconditionally in findfailed_hook() — previously it only caused breakage in combination with --retry-find), see commit:431be2d59bc4677de87f8ee8cc226b58662e63a1.

> * The candidate pics are not saved as artifacts on Jenkins => please file a ticket about it, assigned to me.

Filed as Feature #14830.

#8 Updated by intrigeri 2017-10-15 05:52:13

> Your suspicious from Feature #14814 was not the case; the nature of the error turned out to be much more arcane, about how Java exceptions are integrated into Ruby via rjb. Fixed in commit:f2744487a2e0d4d52554c516e6f1d25d3f3c32e3.

Wow. Glad you found it :)

> While investigating your suspicion, I realized findAny() needed some love too (now that some code runs unconditionally in findfailed_hook() — previously it only caused breakage in combination with --retry-find), see commit:431be2d59bc4677de87f8ee8cc226b58662e63a1.

OK.

It would take me a long time to dive back into all this and do a proper review, but this being said the diff doesn’t look insane. Will test and merge if it works for me :)

#9 Updated by intrigeri 2017-10-16 07:30:58

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

This fixed the OpenPGP applet tests for me on feature/buster + this branch merged. Woohoo! :) Only the two following issues prevent me from merging the updated branch.

On feature/buster + this branch merged, I see a bunch of errors I’ve never seen before e.g.:

    Then I am prompted to configure Electrum                                                                      # features/step_definitions/electrum.rb:44
01:32:40.940001822: Found fuzzy candidate picture for ElectrumNextButton.png with similarity 0.7
    When I create a new bitcoin wallet                                                                            # features/step_definitions/electrum.rb:17
      undefined method `toString' for nil:NilClass (NoMethodError)
      features/electrum.feature:25:in `When I create a new bitcoin wallet'

I saw that in 4 Tor Browser scenarios too. Any chance this is a regression brought by this branch?

One weird thing: apparently we save Sikuli candidates even when we ended up finding the correct picture on screen (and fuzzy image matching is disabled) e.g.:

    When I request a reboot using the emergency shutdown applet                    # features/step_definitions/common_steps.rb:553
      Slept for 5 seconds
02:00:34.252477979: Failed to find fuzzy candidate picture for TailsGreeter.png
02:00:55.235262140: Failed to find fuzzy candidate picture for TailsGreeter.png
02:01:00.013057547: Found fuzzy candidate picture for TailsGreeter.png with similarity 0.8
    Then Tails eventually restarts                                                 # features/step_definitions/common_steps.rb:522

How about we keep and publish these only when there’s a chance we need them?

#10 Updated by intrigeri 2017-10-22 08:25:03

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

Applied in changeset commit:13fe64b2c6cdbfc56d85b23852cc51841c5d9e14.

#11 Updated by intrigeri 2017-10-22 08:40:18

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

(Grml, I guess this will happen every time we merge devel into anything else.)

#12 Updated by intrigeri 2017-11-10 14:16:33

  • Target version changed from Tails_3.3 to Tails_3.5

#13 Updated by anonym 2017-11-30 18:07:32

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

intrigeri wrote:
> This fixed the OpenPGP applet tests for me on feature/buster + this branch merged. Woohoo! :) Only the two following issues prevent me from merging the updated branch.
>
> On feature/buster + this branch merged, I see a bunch of errors I’ve never seen before e.g.:
>
> […]
>
> I saw that in 4 Tor Browser scenarios too. Any chance this is a regression brought by this branch?

Yes! Fixed in commit:6a2ec77c1bb18029bd691ab4799ebd5193f7bb3a.

> One weird thing: apparently we save Sikuli candidates even when we ended up finding the correct picture on screen (and fuzzy image matching is disabled) e.g.:
>
> […]
>
> How about we keep and publish these only when there’s a chance we need them?

That is the intention, so this must be a bug. From your log it’s unclear whether the following step (Then Tails eventually restarts) succeeds or fails, which matters:

On failure: then it is just a matter of logging things incorrectly, which is fixed in commit:afdbec3efe9f32f6facfed31656c9ca96d216ecf.

On success: this could be related to the issue I just fixed with commit:6a2ec77c in a very nasty way: I noticed that using Rjb to directly throwing Java exceptions does nothing. Imagine if the bug that prevents Java exceptions from being re-raised isn’t completely broken, just race:y and sometimes manages to enter the Java exception throwing code; well, since that code does nothing, then findfailed_hook returns, and the Sikuli method succeeds (which is completely incorrect here!), and that is the very last line of code for this whole feature. Any way, with commit:6a2ec77c we avoid both bugs, so this should not be an issue any more.

So it might be fixed. At least I fail to reproduce it myself.

#14 Updated by anonym 2017-12-04 10:04:30

  • Feature Branch changed from test/devel-improvements-from-buster-sprint to test/improvements-from-buster-sprint

Actually, I’d like these changes in the stable branch (waiting to March for all stable-based branches seems like potentially wasteful) so I’ve rebased the branch. Note: this will actually eliminate the test suite delta between stable and devel (the only other change currently in devel being Bug #14772).

I left the original test/devel-improvements-from-buster-sprint around for comparison with this new rebased branch.

#15 Updated by intrigeri 2017-12-04 10:10:11

> Actually, I’d like these changes in the stable branch (waiting to March for all
> stable-based branches seems like potentially wasteful) so I’ve rebased the branch.

Glad I took my sweet time to review then :)

#16 Updated by anonym 2017-12-04 15:01:37

BTW, I can deal with the stabledevel merge, which will contain non-trivial (but still easy, I think) conflicts.

#17 Updated by anonym 2017-12-04 15:06:14

anonym wrote:
> BTW, I can deal with the stabledevel merge, which will contain non-trivial (but still easy, I think) conflicts.

Actually the resolution should simply be: git checkout --theirs

#18 Updated by intrigeri 2017-12-05 14:42:21

  • blocked by Bug #15007: "Pidgin's D-Bus interface is not available" test suite step fails added

#19 Updated by intrigeri 2017-12-05 14:42:42

  • blocked by Bug #15006: Test suite regression: "I successfully start the Unsafe Browser" fails since Tails 3.3 added

#20 Updated by intrigeri 2017-12-05 14:48:00

  • blocked by Bug #14929: 'I can view a file stored in "~/Tor Browser" but not in ~/.gnupg' scenario is broken added

#21 Updated by intrigeri 2017-12-05 14:56:20

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

I’ve run the full test suite on this branch and:

  • lots of failures caused by known recent regressions in our test suite (see tickets I’ve marked as blocking this one); this prevents me from evaluating how this branch works wrt. a baseline since we have no relevant baseline currently. anonym, do you want to batch-fix these issues (on this branch or directly in stable)? Otherwise just tell me and I’ll do it, so I can do this review in better conditions (no strict ETA though, I’ll take it easy :)
  • I can print the current page as "output.pdf" to the default downloads directory' and I can print the current page as "output.pdf" to the persistent Tor Browser directory fail. I’ll investigate but I doubt this has anything to do with this branch.

I still need to verify that the regressions I had previously reported have indeed been fixed by your recent commits. I’m confident!

#22 Updated by intrigeri 2017-12-08 17:57:08

  • blocks Bug #14874: "Failing to add a certificate to Pidgin" test scenario always fails on devel since Sikuli fuzzy matching was implemented added

#23 Updated by intrigeri 2017-12-09 06:23:59

  • blocks deleted (Bug #15006: Test suite regression: "I successfully start the Unsafe Browser" fails since Tails 3.3)

#24 Updated by intrigeri 2017-12-09 06:40:54

  • % Done changed from 0 to 70
  • QA Check changed from Info Needed to Ready for QA

After a full test suite run, I’m happy to tell that the only blocker left is Bug #14929: feel free to merge once you’ve seen that scenario pass on this topic branch.

#25 Updated by anonym 2017-12-09 14:14:03

  • Assignee changed from anonym to intrigeri

[Gah, I forgot to push “Submit” yesterday]

intrigeri wrote:
> * lots of failures caused by known recent regressions in our test suite (see tickets I’ve marked as blocking this one); this prevents me from evaluating how this branch works wrt. a baseline since we have no relevant baseline currently. anonym, do you want to batch-fix these issues (on this branch or directly in stable)? Otherwise just tell me and I’ll do it, so I can do this review in better conditions (no strict ETA though, I’ll take it easy :)

All of these should be fixed now!

> * I can print the current page as "output.pdf" to the default downloads directory' and I can print the current page as "output.pdf" to the persistent Tor Browser directory fail. I’ll investigate but I doubt this has anything to do with this branch.

This is a regression introduced in Tails 3.3, filed as Bug #15024

> I still need to verify that the regressions I had previously reported have indeed been fixed by your recent commits. I’m confident!

Bug #14929 should be fixed in the feature branch now, so I think we’re good to merge, finally!

#26 Updated by intrigeri 2017-12-11 11:20:31

I’ve started another full test suite run (this is @fragile so Jenkins does not help) with a branch based on current stable to double-check.

#27 Updated by intrigeri 2017-12-13 12:53:42

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

#28 Updated by intrigeri 2018-01-04 18:26:37

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

#29 Updated by anonym 2018-01-09 20:55:27

  • Status changed from Fix committed to Resolved