Project

General

Profile

Feature #14576

Feature #14568: Additional Software Packages

Write automated tests for Additional Software GUI (Gherkin)

Added by u over 1 year ago. Updated 7 months ago.

Status:
Resolved
Priority:
Elevated
Assignee:
-
Category:
Test suite
Target version:
Start date:
08/30/2017
Due date:
02/28/2018
% Done:

100%

QA Check:
Pass
Feature Branch:
feature/14596-automated-tests-for-ASP-gui-on-stable
Type of work:
Code
Blueprint:
Starter:
Affected tool:
Additional Software Packages

Description

Please write the Gherkin part of these tests until February 28th 2018. (B3)


Related issues

Blocked by Tails - Feature #14574: Design GUI for Additional Software packages Resolved 01/29/2018
Blocks Tails - Feature #14596: Write automated tests for Additional Software GUI Resolved 09/04/2017 03/06/2018

Associated revisions

Revision 1067fa05 (diff)
Added by bertagaz about 1 year ago

Start writing gherkin scenarios for ASP.

Refs: #14576

Revision e76ef1b3 (diff)
Added by bertagaz about 1 year ago

Mark ASP feature as fragile.

So that it won't be run in Jenkins.

Refs: #14576

Revision a96bf302 (diff)
Added by bertagaz about 1 year ago

ASP test suite: Move Background into a Scenario.

In the same vein than USB upgrade: use a scenario to bootstrap the Tails
environment with ASP persistence configured rather than a Background
step.

Refs: #14576

Revision 4c6cc3cc (diff)
Added by bertagaz about 1 year ago

Typo. (refs: #14576)

Revision 6a078ac8 (diff)
Added by bertagaz about 1 year ago

ASP test suite: rework and simplify failed upgrade scenario.

Refs: #14576

Revision e6920ed3 (diff)
Added by bertagaz about 1 year ago

ASP test suite: Write 2 more scenarios, delay the ASP GUI one for later.

Refs: #14576

Revision f1bdff68 (diff)
Added by bertagaz about 1 year ago

ASP test suite: Fix missing step forgotten while rewriting scenario.

Refs: #14576

Revision 1e5a3276 (diff)
Added by bertagaz about 1 year ago

ASP test suite: Add scenario to test ASP GUI.

Refs: #14576

Revision 4aeb9e8c (diff)
Added by bertagaz about 1 year ago

Test suite: Rework the ASP feature. (refs: #14576)

Revision 7c78ce17 (diff)
Added by bertagaz about 1 year ago

Test suite: reword an ASP scenario name. (refs: #14576)

Revision f2c0aa54 (diff)
Added by bertagaz about 1 year ago

Test suite: fix some misnamed ASP steps. (refs: #14576)

Revision a3b1c395 (diff)
Added by bertagaz about 1 year ago

ASP test suite: typos. (refs #14576)

Revision b40063a4 (diff)
Added by bertagaz about 1 year ago

Rephrase step coherently. (refs: #14576)

Revision 31d6aa8f (diff)
Added by bertagaz about 1 year ago

ASP test suite: typo. (refs: #14576)

Revision 9e4905f1 (diff)
Added by bertagaz about 1 year ago

Test suite: add step to open documentation through ASP notification.

Refs: #14576

Revision 2a39d91d (diff)
Added by bertagaz 8 months ago

Start writing gherkin scenarios for ASP.

Refs: #14576

Revision 50278c0b (diff)
Added by bertagaz 8 months ago

Mark ASP feature as fragile.

So that it won't be run in Jenkins.

Refs: #14576

Revision caad88e0 (diff)
Added by bertagaz 8 months ago

ASP test suite: Move Background into a Scenario.

In the same vein than USB upgrade: use a scenario to bootstrap the Tails
environment with ASP persistence configured rather than a Background
step.

Refs: #14576

Revision f17131ea (diff)
Added by bertagaz 8 months ago

Typo. (refs: #14576)

Revision e764eb66 (diff)
Added by bertagaz 8 months ago

ASP test suite: rework and simplify failed upgrade scenario.

Refs: #14576

Revision 88de8728 (diff)
Added by bertagaz 8 months ago

ASP test suite: Write 2 more scenarios, delay the ASP GUI one for later.

Refs: #14576

Revision eef7ea22 (diff)
Added by bertagaz 8 months ago

ASP test suite: Fix missing step forgotten while rewriting scenario.

Refs: #14576

Revision 47b43e05 (diff)
Added by bertagaz 8 months ago

ASP test suite: Add scenario to test ASP GUI.

Refs: #14576

Revision 59967d5b (diff)
Added by bertagaz 8 months ago

Test suite: Rework the ASP feature. (refs: #14576)

Revision 0592993e (diff)
Added by bertagaz 8 months ago

Test suite: reword an ASP scenario name. (refs: #14576)

Revision 01800025 (diff)
Added by bertagaz 8 months ago

Test suite: fix some misnamed ASP steps. (refs: #14576)

Revision d10bf68b (diff)
Added by bertagaz 8 months ago

ASP test suite: typos. (refs #14576)

Revision 1cadc359 (diff)
Added by bertagaz 8 months ago

Rephrase step coherently. (refs: #14576)

Revision 05d7522e (diff)
Added by bertagaz 8 months ago

ASP test suite: typo. (refs: #14576)

Revision 405b8f4e (diff)
Added by bertagaz 8 months ago

Test suite: add step to open documentation through ASP notification.

Refs: #14576

Revision 0b37319c (diff)
Added by bertagaz 7 months ago

Test suite: s/boot/start in ASP feature.

Refs: #14576

Revision 8817f754 (diff)
Added by bertagaz 7 months ago

Test suite: Abstract a bit more the APT feature. (refs: #14576)

Revision 15385bc6 (diff)
Added by bertagaz 7 months ago

Test suite: Fix APT feature description. (refs: #14576)

Revision 2ee4ff7b (diff)
Added by bertagaz 7 months ago

Test suite: specify which documentation will be opened by clicking on the ASP notification.

Refs: #14576

Revision fb9e9aa8 (diff)
Added by bertagaz 7 months ago

Test suite: Rephrase second ASP scenario title. (refs: #14576)

Revision f7ba7a18 (diff)
Added by bertagaz 7 months ago

Test suite: Document ASP scenarios interdependencies. (refs: #14576)

Revision 28cecc42 (diff)
Added by bertagaz 7 months ago

Test suite: check for Tor leaks for the whole ASP feature.

Refs: #14576

Revision 4ef1d313 (diff)
Added by bertagaz 7 months ago

Test suite: Fix some ASP feature terminology.

Stick to the terms used in our documentation when talking about the
persistence name.

Refs: #14576

Revision 9f81ac6e (diff)
Added by bertagaz 7 months ago

Test suite: Remove more of the remaining "can" in the ASP feature.

Refs: #14576

Revision 53c0babc (diff)
Added by bertagaz 7 months ago

Test suite: Simplify wording a bit. (refs: #14576)

Revision 67635db4 (diff)
Added by bertagaz 7 months ago

Test suite: Small ASP feature wording fix.

s/with no/witout/

Refs: #14576

Revision 79bed243 (diff)
Added by bertagaz 7 months ago

Test suite: ASP scenario title fix.

Refs: #14576

Revision d473d4e9 (diff)
Added by bertagaz 7 months ago

Test suite: Another small wording fix in ASP feature.

s/when a network is available/when online

Refs: #14576

Revision d51b41a3 (diff)
Added by bertagaz 7 months ago

Test suite: Fix another ASP step wording.

'I add a APT source which has the old version of cowsay' sounds better.

Refs: #14576

Revision 9f311515 (diff)
Added by bertagaz 7 months ago

Test suite: Be more specific about the usage of ASP GUI.

Refs: #14576

Revision 6fd0bc9a (diff)
Added by bertagaz 7 months ago

Test suite: Be more coherent on some ASP scenarios titles.

Refs: #14576

Revision 64a556ec (diff)
Added by bertagaz 7 months ago

Test suite: use a different package for ASP 'locked down' persistence scenario

Refs: #14576

Revision f7a42fca (diff)
Added by bertagaz 7 months ago

Test suite: Specify in a comment that we can not reuse the 'sslh' package in all ASP scenarios.

Refs: #14576

Revision 3bc85eb6 (diff)
Added by bertagaz 7 months ago

Test suite: Factorize out some steps of one of the longest ASP scenario.

Refs: #14576

Revision f0cc9d9e (diff)
Added by bertagaz 7 months ago

Test suite: Reword an ASP scenario and one of its step.

Refs: #14576

Revision a3e531ef (diff)
Added by bertagaz 7 months ago

Test suite: Hide lower part of the ASP tests.

We don't need to specify in the feature that we test for the start of
the ASP installation service.

Refs: #14576

Revision 2d906541 (diff)
Added by bertagaz 7 months ago

Test suite: specify what has been run in the 'locked down' persistence ASP scenario.

Refs: #14576

Revision 4718fe59 (diff)
Added by bertagaz 7 months ago

Test suite: clarify the APT source steps in the ASP upgrade scenario.

Refs: #14576

Revision 9679209a (diff)
Added by bertagaz 7 months ago

Test suite: Tentatively try to describe how we ensure the ASP upgrade process fails.

Refs: #14576

Revision ce21c6c8 (diff)
Added by bertagaz 7 months ago

Test suite: bring back the previous (better) logic of the persistence creation step.

Refs: #14576

Revision 1e482009 (diff)
Added by bertagaz 7 months ago

Test suite: Language fix in the APT feature description.

Normalization to be coherent with the way we write it in other features.

Refs: #14576

Revision 04c84aeb (diff)
Added by bertagaz 7 months ago

Test suite: Reword ASP feature title to be more coherent.

Refs: #14576

Revision 16fd60be (diff)
Added by bertagaz 7 months ago

Test suite: Reword the ASP GUi scenario to be more coherent.

Refs: #14576

Revision 2d6b2a9c (diff)
Added by bertagaz 7 months ago

Test suite: fix ASP step name for coherence.

for/that has/ the old cowsay version.

Refs: #14576

Revision 53befdee (diff)
Added by bertagaz 7 months ago

Test suite: another wording fix in ASP feature after review.

updates/upgrades the package in comments for coherence.

Refs: #14576

Revision eec01214 (diff)
Added by bertagaz 7 months ago

Test suite: fix typo in ASP step comment. (refs: #14576)

Revision a4854e89 (diff)
Added by bertagaz 7 months ago

Test suite: Fix wording of APT tweaks in ASP upgrade scenario steps.

Refs: #14576

Revision 18f1f7d8 (diff)
Added by bertagaz 7 months ago

Test suite: remove reference to a ticket about broken Synaptic step.

Have not seen a failure since a long time, seems that this step is
robust now.

Refs: #14576, #12586

Revision 2ebb02f2 (diff)
Added by bertagaz 7 months ago

Test suite: Rename steps containing the wrong "persistence" word in ASP name.

Refs: #14576

Revision 703f18f6 (diff)
Added by bertagaz 7 months ago

Test suite: Move APT update step for old cowsay installation at the right place.

It can't be done at the APT source configuration step, given we
sometimes use it before being logged in the Tails session.

Refs: #14576

Revision d3ee60cc (diff)
Added by bertagaz 7 months ago

Test suite: Mention the blueprint to explain higher level logic of an ASP scenario.

Refs: #14576

Revision 90a5ff78 (diff)
Added by bertagaz 7 months ago

Test suite: Rewording ASP feature steps and scenarios for more coherence.

  • s/ASP/Additional Software/
  • Do not pretend some scenarios check for packages installation when
    they just check for their presence in ASP configuration.

Refs: #14576

Revision c27b4f45 (diff)
Added by intrigeri 7 months ago

Fix typo (refs: #14576)

"software" is an uncountable noun and "Additional software" is how this feature
is called in our user-facing doc.

Revision 9fb5b137 (diff)
Added by intrigeri 7 months ago

Fix grammar (refs: #14576)

Revision 847ac59d (diff)
Added by intrigeri 7 months ago

Improve phrasing (refs: #14576)

Revision 081ee386 (diff)
Added by intrigeri 7 months ago

Turn step into a complete sentence (refs: #14576)

Revision b6acf100 (diff)
Added by intrigeri 7 months ago

Improve phrasing (refs: #14576)

Revision 62158215 (diff)
Added by intrigeri 7 months ago

Add stable HTML anchor we can reference elsewhere (refs: #14576)

Revision 55aa11cc (diff)
Added by intrigeri 7 months ago

Format comments consistently (refs: #14576)

Revision 76bfa0a8 (diff)
Added by intrigeri 7 months ago

Use stable URL (refs: #14576)

Revision f4eeea3b (diff)
Added by intrigeri 7 months ago

Reflow long lines (refs: #14576).

History

#1 Updated by u over 1 year ago

  • Subject changed from Write automated tests for Additional Software GUI to Write automated tests for Additional Software GUI (Gherkin)

#2 Updated by u over 1 year ago

  • Description updated (diff)

#3 Updated by intrigeri over 1 year ago

  • Category set to Test suite

#4 Updated by BitingBird over 1 year ago

  • Target version set to 2018

#5 Updated by u over 1 year ago

  • Affected tool set to Additional Software Packages

#6 Updated by u over 1 year ago

  • Target version changed from 2018 to Tails_3.6

#7 Updated by intrigeri over 1 year ago

  • Blocked by Feature #14574: Design GUI for Additional Software packages added

#8 Updated by bertagaz about 1 year ago

  • Status changed from Confirmed to In Progress
  • % Done changed from 0 to 20
  • QA Check set to Dev Needed
  • Feature Branch set to feature/14576-automated-tests-for-ASP-gui

I've started to redact the scenarios. I've added 9 scenarios I could think of while reading the blueprint. A quick look from someone to see if I did not forget something would be great.

Feature: Additional software packages
  As a Tails user
  I may want to install softwares not shipped in Tails
  And have them installed automatically when I enable persistence in the Greeter

  Scenario: ASP persistence can be set up and used when installing a package in Tails without persistence

  Scenario: Additional software packages are installed even without network

  Scenario: Packages installed with Synaptic and added to ASP are automatically installed

  Scenario: Packages installed with APT but not added to ASP are not automatically installed

  Scenario: Packages installed with ASP are upgraded when a network is available

  # Tests the ASP upgrade process as well as when a previous upgrade failed.
  Scenario: Recovering in offline mode after a previous failed ASP packages upgrade

  Scenario: I am warned I can not use ASP when booting Tails from DVD and installing a package

  # Tests package removal from ASP configuration using ASP GUI
  Scenario: Editing ASP configuration through the GUI and remove a package from the list

  # Tests manual package removal from ASP using APT
  Scenario: Editing ASP configuration through the GUI and remove a package from the list

  Scenario: I am notified when ASP failed to install a package

Of this 9 new scenarios, 6 are already almost written in Gherkin. It brought expected refactoring in the code. More will come when the 4 left will be written. Moving target, so for now no need to look in details the steps wording yet.

#9 Updated by bertagaz about 1 year ago

bertagaz wrote:

I've started to redact the scenarios. I've added 9 scenarios I could think of while reading the blueprint. A quick look from someone to see if I did not forget something would be great.

[...]

Here's an updated version

Feature: Additional software packages
  As a Tails user
  I may want to install softwares not shipped in Tails
  And have them installed automatically when I enable persistence in the Greeter

  Scenario: I am warned I can not use ASP when booting Tails from DVD and installing a package

  Scenario: ASP persistence can be set up and used when installing a package in Tails without persistence

  Scenario: Additional software packages are installed even without network

  Scenario: Packages installed with Synaptic and added to ASP are automatically installed

  Scenario: Packages installed with APT but not added to ASP are not automatically installed

  Scenario: Packages installed with ASP are upgraded when a network is available

  Scenario: Recovering in offline mode after a previous failed ASP upgrade

  Scenario: I am notified when ASP fails to install a package

  Scenario: Removing a package from ASP through APT 

  Scenario: Removing a package from ASP through its GUI

#10 Updated by bertagaz about 1 year ago

  • % Done changed from 20 to 30
  • Feature Branch changed from feature/14576-automated-tests-for-ASP-gui to wip/feature/14576-automated-tests-for-ASP-gui

Pushed a few commits that finish to do a first write of the steps for all the scenarios described above.

I'll let the dust settle a bit before re-reading them and fixing the last issues I'll find, then it'll be ready for review.

#11 Updated by bertagaz about 1 year ago

  • Assignee changed from bertagaz to anonym
  • % Done changed from 30 to 50
  • QA Check changed from Dev Needed to Ready for QA

bertagaz wrote:

I'll let the dust settle a bit before re-reading them and fixing the last issues I'll find, then it'll be ready for review.

I've pushed some commits on the branch, which fix some issues. I think now all the ASP features described in the blueprint are covered, so it's time for the reviewer to have a look at the Gherkin.

I've used the same trick as in the usb upgrade feature: the second scenario bootstraps a Tails with ASP persistence set. I've used scenarios to uninstall packages and remove them from ASP configuration so that we don't have to install X different packages to test ASP features. It's also a good way to simulate a user's Tails ASP persistence life cycle: installing packages and then removing them, and then re-add them.

So what do you think? There may be improvement in the phrasing probably.

#12 Updated by intrigeri about 1 year ago

  • Blocks Feature #14596: Write automated tests for Additional Software GUI added

#13 Updated by alant about 1 year ago

As asked, I had a look at the scenarios.

I think most of the important things are covered. I suggest adding:

Scenario: I am not notified when I install a package and I have a persistent storage but it is not unlocked

Scenario: I am not notified when I upgrade a package that I upgrade a package that was already installed

Scenario: I am not notified when I uninstall a package that is not configured as additional software

This one currently fails, but should not, so it may be worth testing it:

Scenario: Packages I install with packagekit and add to ASP are automatically installed

#14 Updated by bertagaz about 1 year ago

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

#15 Updated by bertagaz about 1 year ago

alant wrote:

As asked, I had a look at the scenarios.

I think most of the important things are covered.

Great.

I suggest adding:

Scenario: I am not notified when I install a package and I have a persistent storage but it is not unlocked

Sure is missing, good catch.

For the following two, I'm unsure. Mainly because the whole feature is adding almost an hour of run time in Jenkins, and there's still one big scenario to write. So I wonder if we should spend time, energy and resources to test such not so important use cases. Also the 'not notified' concept means the scenario will have to wait a certain amount of time without seeing the notification, which will probably add more run time. What do you think?

Scenario: I am not notified when I upgrade a package that I upgrade a package that was already installed

I'm not sure to get this one. If a user try to upgrade a package (that has probably been upgraded automatically), she should not see any notifications?

Scenario: I am not notified when I uninstall a package that is not configured as additional software

And then for this one:

This one currently fails, but should not, so it may be worth testing it:

Scenario: Packages I install with packagekit and add to ASP are automatically installed

Is it possible at the moment to use packagekit in Tails? Are we shipping a tool to use it to install a package? SOrry, I don't know much about it.

#16 Updated by bertagaz about 1 year ago

bertagaz wrote:

alant wrote:

Scenario: I am not notified when I install a package and I have a persistent storage but it is not unlocked

Sure is missing, good catch.

That's 9f99bfbca21415ae8db4482c5be39bbfb50ec357

#17 Updated by alant about 1 year ago

The tests are always failing (https://jenkins.tails.boum.org/job/test_Tails_ISO_feature-14596-automated-tests-for-asp-gui/lastCompletedBuild/cucumberTestReport/), so I don't know how I can rely on them for CI of my work. Will this be fixed soon?

#18 Updated by bertagaz about 1 year ago

  • Target version changed from Tails_3.7 to Tails_3.8

#19 Updated by intrigeri 12 months ago

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

(As per Alan's reply.)

#22 Updated by intrigeri 11 months ago

  • Target version changed from Tails_3.8 to Tails_3.9

#23 Updated by u 11 months ago

  • Priority changed from Normal to Elevated

bertagaz: did you push your modifications? Can you please reply to Alan's question in #17?
The release of this feature is supposed to happen in Tails 3.9, the RC is currently scheduled in 4 weeks (it might be delayed for half a week). Let's try to fix these tests during the next 2 weeks so that Alan can finish his work. Thanks!

#24 Updated by u 9 months ago

Ping, did you push your modifications?

#25 Updated by intrigeri 9 months ago

  • Target version changed from Tails_3.9 to Tails_3.10.1

#26 Updated by bertagaz 9 months ago

u wrote:

Ping, did you push your modifications?

Yes, I've pushed the fixes with 0fdec7a7 a while ago.

#27 Updated by bertagaz 9 months ago

  • Assignee changed from bertagaz to intrigeri
  • % Done changed from 50 to 60
  • QA Check changed from Info Needed to Ready for QA

#28 Updated by intrigeri 8 months ago

  • Feature Branch changed from wip/feature/14576-automated-tests-for-ASP-gui to feature/14596-automated-tests-for-ASP-gui-on-stable

(I assume that's the version we want to review and merge.)

#29 Updated by intrigeri 8 months ago

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

Hi!

Here I'm looking only at the Gherkin changes and I'm ignoring the implementation of steps on purpose.

Wow, it's obvious you've put lots of work into this. Below I have lots of comments but that's quite expected given you did not have much past experience writing Gherkin and English in not your native language; most of them can be solved trivially.

But wait, I suggest you don't jump on fixing them all immediately: I've just noticed that this branch increases the (non-fragile) test suite runtime by 40% (the ASP feature takes longer than the next 3 features that take most time combined). I see that you've mentioned this problem — which was smaller back then — on this ticket a few months ago but I don't recall any follow-up discussion on this topic. I'd rather have discussed this collectively months ago during a team meeting, because this would have informed your work (what should be implemented and how) instead of now that you've invested so much time into implementing all the tests already. But what's done is done, here we are today, so let's have this conversation now.

It's clear to me that this feature cannot justifiably add such a cost (resources, longer feedback loop): IMO 30 minutes would be great; 40 minutes would be good; 50-60 minutes might be acceptable if every test has a well justified cost/benefit; but 1h40 is definitely too much. I think we should do this: first, look for what makes it take so much time and for cheap ways to optimize the implementation, without affecting the nature of the tests at this point (currently there's no cheating in place, which is a very good starting point, we'll see if we can stick to it). If these optimizations cut the cost down to something acceptable, great, we can stop here. Otherwise, for each test: is it worth its current cost? If not, what would be the drawback of taking shortcuts, e.g. merely checking whether the config was updated as expected instead of rebooting? Or shall we remove it entirely or mark it as @too_slow and skip it on Jenkins by default? If you want I can help a bit, e.g. to brainstorm candidate cheap optimizations (once we know what steps take so much time) and to suggest shortcuts (cheating) we could use.

Now comes the review.

About changes that have side effects on unrelated parts of features/*.feature:

  • Regression: the "APT sources are configured correctly" scenario became a bit too low-level for my taste. If I understand correctly, you did this in order to insert "with genuine APT sources" somewhere. IMO that should be passed as part of the "Given I have started Tails" step (e.g. "Given I have started Tails from DVD without network and logged in") so we can go back to a clean scenario like we had before.
  • I'm not a fan of the new I create a persistent partition semantics i.e. the fact this step is not sufficient anymore to… create a persistent partition. This is going to be confusing. Please extract the shared code you need into a function and introduce a new step for the other places where you need to go through the TPS UI without starting it first from the overview, so that the Gherkin keeps making sense.
  • the "Feature: Installing packages through APT" description does not match what it does anymore.

About features/additional_software_packages.feature itself:

  • s/boot/start/, to match the terminology we use in our doc
  • "I can open the documentation from the notification link" is a bit too generic; maybe "I can open the ASP documentation from the notification link"?
  • Any reason not to tag the whole feature check_tor_leaks instead of doing it manually for almost all scenarios? (I don't understand why the scenarios that don't have this tag haven't it)
  • s/Scenario: I can/Scenario: I/ ("can" does not add anything IMO)
  • s/when I install a package in a Tails that has no persistent partition/when installing a package with no persistent partition/ (shorter)
  • In the second scenario, maybe the first 7 steps could be replaced by a single "Given" one, for better readability?
  • The name of the second scenario should reflect the fact it tests not only setting up ASP, but also whether the package is installed upon reboot.
  • The "ASP persistence" and "to ASP configuration" terminology is new and it's not clear to me what that means when reading the Gherkin. Please instead use terminology that's in our doc (ideally) if possible or in the corresponding code.
  • "being annoyed" and "shuts up" don't fit well into the style of our Gherkin.
  • It's not clear to me what ASP has been started for "cowsay" means.
  • If I understand correctly, we start on the __internal drive in order to reuse the already set up persistent ASP configuration (and presumably the cache), as a performance optimization. If it is so, then please document the inter-dependencies between scenarios via comments (see examples in features/usb_upgrade.feature).
  • Related to the above, please document why it matters that we do not use sslh again in further scenarios (it's already installed every time Tails is started).
  • The feature-wide logic (that's complex due to inter-dependencies) would be easier to follow if the "with locked down persistence" scenario used a different package than others.
  • I think "I update APT using Synaptic" is not needed since b34e8a511733a9dbd8718f93f4f28b05e486cbad.
  • s/I confirm when I am asked if I want to add/I accept adding/ (shorter, more readable)
  • s/I deny when I am asked if I want to remove/I refuse removing/
  • I understand the need for the "the additional software package installation service has started" checkpoint before we test whether a package was installed, but I don't think it's worth a step of its own; besides, its name is a bit too close to low-level implementation details for my taste. IMO this would be more readable: the package "cowsay" is not installed after ASP have been installed (repeating oneself is not as big a problem in English as in some other languages so I have to turn off my French speaker's repetition detector ;)
  • s/Packages I uninstall but don't want to remove from ASP are automatically installed/Packages I remove but refuse removing from ASP are still automatically installed/ ("remove" is the terminology used both in the APT doc and in our own; "still" expresses why it makes sense to test this, just like in another scenario you correctly wrote "anymore"; "refuse" describes the action taken by the user in a clearer way than "don't want", that expresses intent but not necessarily imply that action was actually taken)
  • s/when a network is available/when online/ (whether a network is available does not imply Tails is configured to use it)
  • In "Scenario: Packages I have installed and added to ASP are upgraded when a network is available":
    • s/I configure APT with a custom source for the old version of cowsay/I add a APT source which has the old version of cowsay/
    • In "Packages I have installed and added to ASP are upgraded when a network is available", I don't understand "And then to remove it so that cowsay gets updated". Why wouldn't APT upgrade the package, as long as a newer version is available? Is that caused by our APT pinning configuration?
  • "ASP packages" is redundant. You can just say ASP or (to match our doc) "additional software".
  • The phrasing of I remove "cowsay" from the list of ASP packages made me wonder if that's done by directly modifying the config file or via some GUI. I suggest instead: I remove "cowsay" from the list of ASP using Additional Software.
  • I'm not sure what "I can open the ASP configuration" means.
  • From reading the Gherkin, "Scenario: Recovering in offline mode after ASP previously failed to upgrade a package" seems to do what we want (and in a more clever way that what I suggested on the blueprint), but I had to manually reverse-engineer the high-level logic in order to compare it to https://tails.boum.org/blueprint/additional_software_packages/offline_mode/. Please make the high-level logic clear, starting with making it explicit that "APT indices are successfully updated" (see the blueprint for context), possibly by rephrasing the too vague "I prepare the ASP upgrade process to fail".

#30 Updated by intrigeri 8 months ago

intrigeri wrote:

I think we should do this: first, look for what makes it take so much time and for cheap ways to optimize the implementation, without affecting the nature of the tests at this point (currently there's no cheating in place, which is a very good starting point, we'll see if we can stick to it). If these optimizations cut the cost down to something acceptable, great, we can stop here.

Forgot to say: IMO this first part will be better tracked on the ticket about the implementation because it should not affect the Gherkin text at all.

#31 Updated by bertagaz 8 months ago

  • Assignee changed from bertagaz to intrigeri
  • QA Check changed from Dev Needed to Info Needed

intrigeri wrote:

Here I'm looking only at the Gherkin changes and I'm ignoring the implementation of steps on purpose.

Wow, it's obvious you've put lots of work into this.

Yes, I've tried to cover every use cases and the whole code base.

But wait, I suggest you don't jump on fixing them all immediately: I've just noticed that this branch increases the (non-fragile) test suite runtime by 40% (the ASP feature takes longer than the next 3 features that take most time combined). I see that you've mentioned this problem — which was smaller back then — on this ticket a few months ago but I don't recall any follow-up discussion on this topic. I'd rather have discussed this collectively months ago during a team meeting, because this would have informed your work (what should be implemented and how) instead of now that you've invested so much time into implementing all the tests already. But what's done is done, here we are today, so let's have this conversation now. [...]

Moved this to #14596#note-40

Now comes the review.

Thanks for this extensive review.

I'm only replying where it makes sense and have something to say rather than fix it.

About features/additional_software_packages.feature itself:

  • Any reason not to tag the whole feature check_tor_leaks instead of doing it manually for almost all scenarios? (I don't understand why the scenarios that don't have this tag haven't it)

Some of the scenarios don't involve network at all, so they are not marked with check_tor_leaks.

  • It's not clear to me what ASP has been started for "cowsay" means.

I use that to grep in the logs if ASP is mentionning the right package. But maybe indeed it doesn't have much to do with what the scenario is testing, and I can get rid of this trick, and just check of ASP has just been started?

  • If I understand correctly, we start on the __internal drive in order to reuse the already set up persistent ASP configuration (and presumably the cache), as a performance optimization.

So there's a bit of optimization in the end. :)

  • Related to the above, please document why it matters that we do not use sslh again in further scenarios (it's already installed every time Tails is started).

We use it once, to test if it's not removed when denying the removal of it in ASP conf. But you're right, ot's worth mentionning in a comment.

I'll try without (which would remove a bit of run time).

  • In "Scenario: Packages I have installed and added to ASP are upgraded when a network is available":
    • In "Packages I have installed and added to ASP are upgraded when a network is available", I don't understand "And then to remove it so that cowsay gets updated". Why wouldn't APT upgrade the package, as long as a newer version is available? Is that caused by our APT pinning configuration?

Yes. Do you mean I can just remove the pinning?

#32 Updated by intrigeri 7 months ago

  • Assignee changed from intrigeri to bertagaz
  • QA Check changed from Info Needed to Dev Needed

intrigeri wrote:

  • Any reason not to tag the whole feature check_tor_leaks instead of doing it manually for almost all scenarios? (I don't understand why the scenarios that don't have this tag haven't it)

Some of the scenarios don't involve network at all, so they are not marked with check_tor_leaks.

I see. My concern is that next time someone adds a scenario in this feature, they might forget this tag. Which would be no problem whatsoever if the tag was set globally. Now, I don't remember if check_tor_leaks breaks stuff for scenarios with networking disabled.

  • It's not clear to me what ASP has been started for "cowsay" means.

I use that to grep in the logs if ASP is mentionning the right package. But maybe indeed it doesn't have much to do with what the scenario is testing, and I can get rid of this trick, and just check of ASP has just been started?

It seems to me that the whole point of the scenario is to check that there's no notification in this specific situation. So just checking that ASP has started would make the entire scenario kind of pointless.

To clarify, my concern was about the phrasing: I litteraly don't know what ASP has been started for "cowsay" means.

  • In "Scenario: Packages I have installed and added to ASP are upgraded when a network is available":
    • In "Packages I have installed and added to ASP are upgraded when a network is available", I don't understand "And then to remove it so that cowsay gets updated". Why wouldn't APT upgrade the package, as long as a newer version is available? Is that caused by our APT pinning configuration?

Yes. Do you mean I can just remove the pinning?

I did not mean to suggest a specific solution: I did not understand the explanation this comment seems to convey (and still don't understand it). So please check that this comment is correct (i.e. that we need to "remove the custom APT source for the old cowsay version") and then if it's wrong, drop the useless step, and if it's right, explain why in the comment :)

#33 Updated by bertagaz 7 months ago

I'll start fixing things raised by this review this week.

#35 Updated by bertagaz 7 months ago

I've pushed a bunch of commits that fix the following checked parts of your review. More to come...

intrigeri wrote:

Now comes the review.

About changes that have side effects on unrelated parts of features/*.feature:

  • Regression: the "APT sources are configured correctly" scenario became a bit too low-level for my taste. If I understand correctly, you did this in order to insert "with genuine APT sources" somewhere. IMO that should be passed as part of the "Given I have started Tails" step (e.g. "Given I have started Tails from DVD without network and logged in") so we can go back to a clean scenario like we had before.
  • I'm not a fan of the new I create a persistent partition semantics i.e. the fact this step is not sufficient anymore to… create a persistent partition. This is going to be confusing. Please extract the shared code you need into a function and introduce a new step for the other places where you need to go through the TPS UI without starting it first from the overview, so that the Gherkin keeps making sense.
  • the "Feature: Installing packages through APT" description does not match what it does anymore.

About features/additional_software_packages.feature itself:

  • s/boot/start/, to match the terminology we use in our doc
  • "I can open the documentation from the notification link" is a bit too generic; maybe "I can open the ASP documentation from the notification link"?
  • Any reason not to tag the whole feature check_tor_leaks instead of doing it manually for almost all scenarios? (I don't understand why the scenarios that don't have this tag haven't it)

That seems to work when the whole feature is tagged, so I've done so.

  • s/Scenario: I can/Scenario: I/ ("can" does not add anything IMO)

I've also removed a bunch of other "can" here and there.

  • s/when I install a package in a Tails that has no persistent partition/when installing a package with no persistent partition/ (shorter)
  • In the second scenario, maybe the first 7 steps could be replaced by a single "Given" one, for better readability?
  • The name of the second scenario should reflect the fact it tests not only setting up ASP, but also whether the package is installed upon reboot.
  • The "ASP persistence" and "to ASP configuration" terminology is new and it's not clear to me what that means when reading the Gherkin. Please instead use terminology that's in our doc (ideally) if possible or in the corresponding code.
  • "being annoyed" and "shuts up" don't fit well into the style of our Gherkin.
  • It's not clear to me what ASP has been started for "cowsay" means.

Tentatively reworded it then.

  • If I understand correctly, we start on the __internal drive in order to reuse the already set up persistent ASP configuration (and presumably the cache), as a performance optimization. If it is so, then please document the inter-dependencies between scenarios via comments (see examples in features/usb_upgrade.feature).
  • Related to the above, please document why it matters that we do not use sslh again in further scenarios (it's already installed every time Tails is started).
  • The feature-wide logic (that's complex due to inter-dependencies) would be easier to follow if the "with locked down persistence" scenario used a different package than others.
  • I think "I update APT using Synaptic" is not needed since b34e8a511733a9dbd8718f93f4f28b05e486cbad.

I don't know why but it breaks if we remove this step, so I've left it. Given I've spent more time on this feature than what was planned on this feature, I won't try to fix that.

  • s/I confirm when I am asked if I want to add/I accept adding/ (shorter, more readable)
  • s/I deny when I am asked if I want to remove/I refuse removing/
  • I understand the need for the "the additional software package installation service has started" checkpoint before we test whether a package was installed, but I don't think it's worth a step of its own; besides, its name is a bit too close to low-level implementation details for my taste. IMO this would be more readable: the package "cowsay" is not installed after ASP have been installed (repeating oneself is not as big a problem in English as in some other languages so I have to turn off my French speaker's repetition detector ;)
  • s/Packages I uninstall but don't want to remove from ASP are automatically installed/Packages I remove but refuse removing from ASP are still automatically installed/ ("remove" is the terminology used both in the APT doc and in our own; "still" expresses why it makes sense to test this, just like in another scenario you correctly wrote "anymore"; "refuse" describes the action taken by the user in a clearer way than "don't want", that expresses intent but not necessarily imply that action was actually taken)
  • s/when a network is available/when online/ (whether a network is available does not imply Tails is configured to use it)
  • In "Scenario: Packages I have installed and added to ASP are upgraded when a network is available":
    • s/I configure APT with a custom source for the old version of cowsay/I add a APT source which has the old version of cowsay/
    • In "Packages I have installed and added to ASP are upgraded when a network is available", I don't understand "And then to remove it so that cowsay gets updated". Why wouldn't APT upgrade the package, as long as a newer version is available? Is that caused by our APT pinning configuration?

Tried to clarify what the steps really means then.

  • "ASP packages" is redundant. You can just say ASP or (to match our doc) "additional software".
  • The phrasing of I remove "cowsay" from the list of ASP packages made me wonder if that's done by directly modifying the config file or via some GUI. I suggest instead: I remove "cowsay" from the list of ASP using Additional Software.
  • I'm not sure what "I can open the ASP configuration" means.
  • From reading the Gherkin, "Scenario: Recovering in offline mode after ASP previously failed to upgrade a package" seems to do what we want (and in a more clever way that what I suggested on the blueprint), but I had to manually reverse-engineer the high-level logic in order to compare it to https://tails.boum.org/blueprint/additional_software_packages/offline_mode/. Please make the high-level logic clear, starting with making it explicit that "APT indices are successfully updated" (see the blueprint for context), possibly by rephrasing the too vague "I prepare the ASP upgrade process to fail".

OK, I've added a comment trying to explain this higher logic. Is that clearer?

#36 Updated by intrigeri 7 months ago

  • Target version changed from Tails_3.10.1 to Tails_3.11

#37 Updated by bertagaz 7 months ago

I've checked in my previous note what I've been implemented since I've posted it. Getting closer to fix them all.

#38 Updated by u 7 months ago

bertagaz wrote:

I've checked in my previous note what I've been implemented since I've posted it. Getting closer to fix them all.

Thanks. So we're left with

  • I think "I update APT using Synaptic" is not needed since b34e8a511733a9dbd8718f93f4f28b05e486cbad. and
  • I'm not a fan of the new I create a persistent partition semantics i.e. the fact this step is not sufficient anymore to… create a persistent partition. This is going to be confusing. Please extract the shared code you need into a function and introduce a new step for the other places where you need to go through the TPS UI without starting it first from the overview, so that the Gherkin keeps making sense.

#39 Updated by bertagaz 7 months ago

u wrote:

Thanks. So we're left with

  • I think "I update APT using Synaptic" is not needed since b34e8a511733a9dbd8718f93f4f28b05e486cbad. and
  • I'm not a fan of the new I create a persistent partition semantics i.e. the fact this step is not sufficient anymore to… create a persistent partition. This is going to be confusing. Please extract the shared code you need into a function and introduce a new step for the other places where you need to go through the TPS UI without starting it first from the overview, so that the Gherkin keeps making sense.

Yes. I've pushed a commit for the last item of this list, and am waiting for feedbacks from Jenkins. So far it seems to work. The former of this list I've already answered and will leave it as it is.

#40 Updated by bertagaz 7 months ago

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

bertagaz wrote:

Yes. I've pushed a commit for the last item of this list, and am waiting for feedbacks from Jenkins. So far it seems to work. The former of this list I've already answered and will leave it as it is.

Indeed it does, so I think this ticket is ready for the second round of review.

#41 Updated by intrigeri 7 months ago

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

Follow-ups on the previous review, your fixes and answers:

  • Terminology:
    • I see you're replaced the "ASP persistence" neologism with another one: "Additional Software persistence". That does not address the concern I've raised. See https://tails.boum.org/doc/first_steps/additional_software/.
    • Same for "Additional Software persistence is correctly configured for package" and "is not part of Additional Software persistence configuration", see the terminology used in the user-facing software and the doc, that uses "add to your additional software" and "your list of additional software" so the test suite should say "$package is in the list of additional software" etc.
    • Wrt. "Additional software packages" and "ASP": "Additional software packages" was an internal name for this project but the user-facing feature is called "additional software". Would be nice if the test suite acknowledged it (BDD spirit and all that).
  • Wrt. "I update APT using Synaptic": fair enough if this breakage is really a test suite problem and not an actual bug that the test suite discovered, that we should not paper over. I'd be satisfied enough if you tell me that the same scenario does work fine without this manual update step when you go through it by hand.
  • There's still one occurrence of "the additional software package upgrade service has started".
  • Wrt. "I remove the custom APT source for the old cowsay version", I've looked at the whole thing again and I think I now understand (a comment initially sent me on a wrong path for a while, but nothing made sense to I ended up reading the step definitions, which unblocked me). So, both the "add a APT source" and "remove the custom APT source" steps also add/remove the pinning. OOOOK. This was not obvious when reading the step names and the comments above them (granted, there's one place that says "pin it" but it's 10 lines above the part that I was concerned about, and all the other "I add a APT source" instances have a misleading name and comment). I think I would not have spent so much time understanding what was going on if these steps were renamed to "I configure APT to prefer an old version of cowsay" and "I revert the APT teaks that made it prefer an old version of cowsay". Alternatively, split each of the existing "add a APT source" and "remove the custom APT source" steps into two steps, one about the APT source and the other about the pinning. And as part of this clarification, IMO the APT tweaks shall be done in steps that are explicitly used in Gherkin, not hidden in "I install an old version": the fact we need a 2-lines comment to explain what this step does (i.e. not what it says) turns on red lights. And then you can probably drop quite a few comments, which is a good sign wrt. the quality of the Gherkin text :)
  • Wrt. the high-level view of "Scenario: Recovering in offline mode after ASP previously failed to upgrade a package": thanks for the comments, they do help! I would argue that needing comments to explain the high-lever logic is a bad sign regarding the quality of the Gherkin, but fixing this would require a process closer to mentoring than to reviewing, so let's say it's good enough ⇒ please add a link to the page that has the high-level explanation already and let's leave it at that.

Other things I'd really like to see fixed:

  • I think "Packages I uninstall through ASP GUI" is misleading: last time I checked, that GUI did not uninstall packages.
  • In "I remove the custom APT source for the old cowsay version", s/for/that has/ would make it clearer and consistent with how you wrote the same thing elsewhere in this feature.
  • "offlical" ← typo
  • In "updates the package", s/updates/upgrades/ to be consistent with the terminology you use everywhere else.
  • The Synaptic scenario on stable is flagged fragile due to #12586. The new one still references that ticket but is not marked fragile so I'm not sure what's the status. If you're confident it's not fragile anymore, fine, then drop that reference and close the ticket. Otherwise, flag it as fragile?
  • The title of a few scenarios that say "are automatically installed", "installed anymore" or similar, do not match what they actually test.

Could be seen as nitpicking, happy to fix those myself if you prefer:

  • In "Feature: APT sources must be correctly configured", s/must be/are/, to match a little bit better the style of our Gherkin. (Now of course, this feature does not really qualify as a feature anymore since it does not describes behavior a user cares about, but well, good enough!)

#42 Updated by bertagaz 7 months ago

Thanks for the review. The size of it is a bit scary though and I wonder how much review passes like this we'll have...

#43 Updated by intrigeri 7 months ago

Thanks for the review. The size of it is a bit scary though and I wonder how much review passes like this we'll have...

I certainly hope that the next one will be the last.

#44 Updated by bertagaz 7 months ago

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

I believe I've fixed all I could. Here's some remarks:

intrigeri wrote:

Same for "Additional Software persistence is correctly configured for package" and "is not part of Additional Software persistence configuration", see the terminology used in the user-facing software and the doc, that uses "add to your additional software" and "your list of additional software" so the test suite should say "$package is in the list of additional software" etc.

OK I've removed the "persistence" word, and any mention of "ASP" then. I did not entirely renamed all the steps as you proposed. For the one testing if the package is added to the ASP list, it also tests other different things, and not just the presence of the package name in the list. So it makes more sense to talk about ASP configuration than simply the list.

intrigeri wrote:

Wrt. "I update APT using Synaptic": fair enough if this breakage is really a test suite problem and not an actual bug that the test suite discovered, that we should not paper over. I'd be satisfied enough if you tell me that the same scenario does work fine without this manual update step when you go through it by hand.

Given the tight timeline, I'm not sure I'll have time to test that. I that wasn't a test suite problem, I believe we'd have notice that or had feedbacks from users.

intrigeri wrote:

There's still one occurrence of "the additional software package upgrade service has started".

I've leaved this one intentionnaly as a trade off between good looking Gerkhin and additional complexity in the step definition. Hiding this step would make the 'the package "cowsay" installed version is newer than "3.03+dfsg2-1"' step code much more complex as we're already testing for the starting of the installation service at different times, and we'd have to decouple the step to also be able to test for the ASP upgrade service starting in one case only. So I'm not really sure it's worth it.

#45 Updated by bertagaz 7 months ago

bertagaz wrote:

intrigeri wrote:

Wrt. "I update APT using Synaptic": fair enough if this breakage is really a test suite problem and not an actual bug that the test suite discovered, that we should not paper over. I'd be satisfied enough if you tell me that the same scenario does work fine without this manual update step when you go through it by hand.

Given the tight timeline, I'm not sure I'll have time to test that. I that wasn't a test suite problem, I believe we'd have notice that or had feedbacks from users.

While working on #14596 I found the time to test that and it works manually.

#46 Updated by intrigeri 7 months ago

bertagaz wrote:

intrigeri wrote:

Wrt. "I update APT using Synaptic": fair enough if this breakage is really a test suite problem and not an actual bug that the test suite discovered, that we should not paper over. I'd be satisfied enough if you tell me that the same scenario does work fine without this manual update step when you go through it by hand.

Given the tight timeline, I'm not sure I'll have time to test that. I that wasn't a test suite problem, I believe we'd have notice that or had feedbacks from users.

While working on #14596 I found the time to test that and it works manually.

Excellent! Glad we have no non-test-suite bug here :)

#47 Updated by intrigeri 7 months ago

  • Status changed from In Progress to Resolved
  • QA Check changed from Ready for QA to Pass

bertagaz wrote:

I believe I've fixed all I could.

Great, LGTM!

intrigeri wrote:

Same for "Additional Software persistence is correctly configured for package" and "is not part of Additional Software persistence configuration", see the terminology used in the user-facing software and the doc, that uses "add to your additional software" and "your list of additional software" so the test suite should say "$package is in the list of additional software" etc.

OK I've removed the "persistence" word, and any mention of "ASP" then. I did not entirely renamed all the steps as you proposed. For the one testing if the package is added to the ASP list, it also tests other different things, and not just the presence of the package name in the list. So it makes more sense to talk about ASP configuration than simply the list.

OK, fair enough. But then the name of the corresponding scenarios ("configured in the Additional Software list" etc.) is neither consistent with the approach you've picked (it does not express the subtlety you just explained) nor with list terminology (add to / remove from). Not a blocker.

intrigeri wrote:
intrigeri wrote:

There's still one occurrence of "the additional software package upgrade service has started".

I've leaved this one intentionnaly as a trade off between good looking Gerkhin and additional complexity in the step definition. Hiding this step would make the 'the package "cowsay" installed version is newer than "3.03+dfsg2-1"' step code much more complex as we're already testing for the starting of the installation service at different times, and we'd have to decouple the step to also be able to test for the ASP upgrade service starting in one case only. So I'm not really sure it's worth it.

OK.

I've pushed a few targeted fixes and phrasing improvements that don't deserve another Redmine round trip so I can now close this ticket.

#48 Updated by intrigeri 7 months ago

  • Assignee deleted (intrigeri)
  • % Done changed from 60 to 100

Also available in: Atom PDF