Tails

#1 Updated by Anonymous 2017-08-30 16:24:52

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

#2 Updated by Anonymous 2017-08-30 16:25:14

• Description updated

#3 Updated by intrigeri 2017-08-30 16:43:34

• Category set to Test suite

#4 Updated by BitingBird 2017-08-30 17:11:44

• Target version set to 2018

#5 Updated by Anonymous 2017-09-04 16:51:37

• Affected tool set to Additional Software Packages

#6 Updated by Anonymous 2017-09-07 08:30:02

• Target version changed from 2018 to Tails_3.6

#7 Updated by intrigeri 2017-09-29 17:50:43

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

#8 Updated by bertagaz 2018-02-25 18:16:05

• 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.

#10 Updated by bertagaz 2018-02-26 11:19:04

• % 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 2018-03-04 12:30:57

• 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 2018-03-05 13:10:09

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

#14 Updated by bertagaz 2018-03-14 11:32:25

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

#18 Updated by bertagaz 2018-05-10 11:09:24

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

#19 Updated by intrigeri 2018-05-25 13:21:53

• 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 2018-06-26 16:28:00

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

#23 Updated by Anonymous 2018-07-05 12:24:49

• 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!

#25 Updated by intrigeri 2018-09-05 16:26:59

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

#27 Updated by bertagaz 2018-09-08 06:00:37

• 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 2018-09-15 07:46:06

• 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 2018-09-15 10:09:03

• 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 commit: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”.

#31 Updated by bertagaz 2018-09-21 12:28:19

• 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 Feature #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 think “I update APT using Synaptic” is not needed since commit:b34e8a511733a9dbd8718f93f4f28b05e486cbad.

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 2018-10-10 21:46:21

• 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 :)

#36 Updated by intrigeri 2018-10-24 17:03:41

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

#38 Updated by Anonymous 2018-10-31 15:47:40

• Deliverable for changed from 299 to 319

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.

#40 Updated by bertagaz 2018-11-01 09:25:29

• 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 2018-11-01 16:28:58

• 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 Bug #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!)

#44 Updated by bertagaz 2018-11-07 10:54:21

• 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.

#47 Updated by intrigeri 2018-11-08 10:21:58

• 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 2018-11-08 10:22:19

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