Feature #14572

Implement automated tests for Additional Software/Offline Mode

Added by Anonymous 2017-08-30 13:11:48 . Updated 2018-03-14 11:12:34 .

Status:
Resolved
Priority:
Normal
Assignee:
Category:
Target version:
Tails_3.6
Start date:
2017-08-30
Due date:
2018-01-15
% Done:

100%

Feature Branch:
feature/14572-automated-tests-for-ASP-offline-mode
Type of work:
Code
Blueprint:

https://tails.boum.org/blueprint/additional_software_packages_offline_mode/

Starter:
Affected tool:
Additional Software Packages
Deliverable for:
299

Description

To be done before 15th January 2018. (B3)

Subtasks

Related issues

Related to Tails - Feature #14571: Code review for Implement Offline Mode Resolved 2017-08-30 2018-01-15
Related to Tails - Bug #15799: "Additional software packages are installed even without network" test always fail in my environment Resolved 2018-08-17
Blocks Tails - Bug #15198: Convert ASP to Python3 and follow PEP-8 Resolved 2018-01-19
Blocks Tails - Feature #14570: Implement Offline Mode for Additional Software Packages Resolved 2016-01-17

History

#1 Updated by Anonymous 2017-08-30 13:13:37

#2 Updated by Anonymous 2017-08-30 13:13:59

  • Due date set to 2018-01-15

#3 Updated by Anonymous 2017-08-30 13:15:00

  • Description updated

#4 Updated by Anonymous 2017-08-30 13:15:16

  • Description updated

#6 Updated by Anonymous 2017-08-30 13:16:07

  • related to Feature #14571: Code review for Implement Offline Mode added

#7 Updated by Anonymous 2017-08-30 13:16:28

  • related to Feature #14570: Implement Offline Mode for Additional Software Packages added

#8 Updated by Anonymous 2017-08-30 13:16:40

#9 Updated by Anonymous 2017-08-30 16:24:18

  • Type of work changed from Sysadmin to Code

#10 Updated by intrigeri 2017-08-30 17:27:05

#11 Updated by Anonymous 2017-09-04 16:50:47

  • Affected tool set to Additional Software Packages

#12 Updated by sajolida 2017-09-06 16:24:21

mid-January is 3.4 and not 3.3 so maybe there’s a mistake in the target version…

#13 Updated by Anonymous 2017-09-06 20:46:20

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

thanks, sajolida!

#14 Updated by intrigeri 2017-10-01 08:14:14

  • Status changed from Confirmed to In Progress

Applied in changeset commit:995662290f3de206243a95812b19ef4b984cf776.

#15 Updated by intrigeri 2017-12-14 12:34:45

  • Blueprint set to https://tails.boum.org/blueprint/additional_software_packages_offline_mode/

We are documenting on the blueprint the potentially problematic cases that were fixed already or will be fixed. This should be a good basis for writing these new test cases :)

#16 Updated by intrigeri 2017-12-15 17:32:21

The only remaining TBD item on the code side (Feature #14570) is a minor refactoring, so you can get started as soon as you want, based on the corresponding topic branch. We will wait for the automated tests you’ll write before merging the branch, in order to ensure it works well. I’ve stuffed out the blueprint a bit so it’s easier for you to understand what potential failure modes matter and should be tested.

Thanks in advance and happy test writing!

#17 Updated by intrigeri 2017-12-15 17:32:50

  • Status changed from In Progress to Confirmed

#18 Updated by Anonymous 2018-01-05 14:11:06

alan says: there are no tests for additional software pakcages yet, so you need to implement basic tests first, which will make sure we introduce no regressions

#19 Updated by Anonymous 2018-01-15 14:40:32

Hey bertagaz: the deadline for this work was supposed to be today. Did you manage to work on this? If not, what are the blockers?

#20 Updated by bertagaz 2018-01-16 15:33:13

  • Assignee changed from bertagaz to anonym
  • % Done changed from 0 to 50
  • QA Check set to Ready for QA
  • Feature Branch set to feature/14572-automated-tests-for-ASP-offline-mode

u wrote:
> Hey bertagaz: the deadline for this work was supposed to be today. Did you manage to work on this? If not, what are the blockers?

Yes I did, but some tiny subtilities took me a bit more time than I thought to get around (plus helping a bit with the emergency release last week). I’ve pushed a branch which is based on the one from Bug #9059 with Feature #14570 merged in. The merge was not so easy, took me a bit of time too.

Anyway, it implements testing of additional software packages in offline mode based on the improvements made on the other branches. Works well here, I think it’s ok to be reviewed, so assigning to anonym. Sorry for the slight delaying.

#21 Updated by bertagaz 2018-01-16 15:38:14

  • Status changed from Confirmed to In Progress

Applied in changeset commit:53bdfcdb7d0f40f021a94fb0b7dbfcf9ab5f6761.

#22 Updated by Anonymous 2018-01-16 16:54:48

bertagaz wrote:
> Applied in changeset commit:53bdfcdb7d0f40f021a94fb0b7dbfcf9ab5f6761.

Thanks!

#23 Updated by intrigeri 2018-01-16 16:57:52

> branch which is based on the one from Bug #9059 with Feature #14570 merged in. The merge was not so easy, took me a bit of time too.

Ouch. I think we need Alan to either validate this merge or to re-do it and compare the result: IMO the main developer of this code is responsible (and better placed) for resolving merge conflicts between his own branches. Now, of course the timing depends on what part of Alan’s work will be deemed not too invasive & risky for a bugfix release.

#24 Updated by bertagaz 2018-01-16 17:59:24

intrigeri wrote:
> > branch which is based on the one from Bug #9059 with Feature #14570 merged in. The merge was not so easy, took me a bit of time too.
>
> Ouch. I think we need Alan to either validate this merge or to re-do it and compare the result: IMO the main developer of this code is responsible (and better placed) for resolving merge conflicts between his own branches. Now, of course the timing depends on what part of Alan’s work will be deemed not too invasive & risky for a bugfix release.

That’d be nice for sure. If he cannot, I’ve spent quite some time on it, carefully reading both branches changes several times comparing the merge result and ensuring nothing was left appart. So maybe if the reviewer does the same thing, it could be OK too.

#25 Updated by alant 2018-01-16 23:38:21

intrigeri wrote:
> > branch which is based on the one from Bug #9059 with Feature #14570 merged in. The merge was not so easy, took me a bit of time too.

I don’t understand why the testing branch should be based on these branches.

> Ouch. I think we need Alan to either validate this merge or to re-do it and compare the result: IMO the main developer of this code is responsible (and better placed) for resolving merge conflicts between his own branches. Now, of course the timing depends on what part of Alan’s work will be deemed not too invasive & risky for a bugfix release.

I checked the merge and it looks OK to me. I didn’t test though.

#26 Updated by intrigeri 2018-01-17 06:46:27

>> > branch which is based on the one from Bug #9059 with Feature #14570 merged in. The merge was not so easy, took me a bit of time too.

> I don’t understand why the testing branch should be based on these branches.

Indeed, in theory only the branch for Feature #14570 is relevant here, but given how deeply the integration is being reworked on Bug #9059, I think it totally makes sense to base the tests for offline mode on it to avoid having to rework them later (e.g. in case the tests have to do stuff behind the hood that depends on implementation details, which is generally a bad idea but sometimes there’s no avoiding it).

#27 Updated by anonym 2018-01-22 08:49:16

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

#28 Updated by anonym 2018-01-30 16:56:54

  • Assignee changed from anonym to bertagaz
  • % Done changed from 50 to 70

Works great! I’ve pushed quite a few style fixes for you to review, and ended up doing some work to improve path globbing (over the guest fs) in this branch. Personally I’m happy to have this branch merged for Tails 3.6.

#29 Updated by Anonymous 2018-02-05 13:58:01

  • blocks Bug #15198: Convert ASP to Python3 and follow PEP-8 added

#30 Updated by intrigeri 2018-02-08 06:59:42

  • related to deleted (Feature #14570: Implement Offline Mode for Additional Software Packages)

#31 Updated by intrigeri 2018-02-08 06:59:50

  • blocks Feature #14570: Implement Offline Mode for Additional Software Packages added

#32 Updated by bertagaz 2018-02-20 20:14:24

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

Applied in changeset commit:7dfa97612868f7459f79cab29a66114969e18658.

#33 Updated by bertagaz 2018-02-20 20:27:39

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

anonym wrote:
> Works great! I’ve pushed quite a few style fixes for you to review, and ended up doing some work to improve path globbing (over the guest fs) in this branch. Personally I’m happy to have this branch merged for Tails 3.6.

So does it, it’s merged into devel.

#34 Updated by intrigeri 2018-02-21 08:54:43

  • Status changed from Fix committed to In Progress
  • Assignee set to bertagaz
  • QA Check changed from Pass to Info Needed

Hi bertagaz and anonym!

This popped up on my radar when looking at recent changes on the devel branch (which I do before merging to my local devel); I had not looked at it earlier because 1. it’s not my job; 2. I don’t want to be a control freak by default; 3. I had made my expectations clear during an ASP meeting in December or January and bertagaz had said he had taken note of them.

I’ve read the diff and commit messages a few times and I don’t understand why we do the “I configure APT to use non-onion sources” dance. It’s questionable to test something different than what users do but I assume there’s a good reason for that. However I am not able to guess what that reason is so could you please add a comment that makes this clear? Thanks in advance!

Also, it seems to me that the added test only exercises very basic usage of ASP in offline mode, and in particular only one of the 3 failure modes we’ve identified on the blueprint, i.e. “One of the packages was not cached in the first place” that was fixed in 3.5. I see no explanation for not testing the other failure modes, so here we go. This test suite checks neither:

  • “APT indices have expired”: I think it’s acceptable because I don’t think “APT checks the indices expiration date only when it downloads them, not when it reads them to install packages” will ever change silently; let’s hope someone will notice the change in APT’s changelog if this ever changes (and I assume there will be a config setting available to switch back to the old behaviour). So in this case I don’t think the cost/benefit of writing a test for that potential failure mode is worth it :)
  • “Incomplete online upgrade process” (Feature #14570) whose fix was merged at the same time as this branch for 3.6. That’s the only updated piece of code we have for Feature #14570 so as I made clear in our December (or January?) meeting, I would expect us to exercise it. I understand it may be non-trivial to do so. I could live with a conclusion of “the cost/benefit is not worth it” if that’s the case, but I think there needs to be a clear reasoning that leads to that conclusion.

#35 Updated by bertagaz 2018-02-21 12:14:57

intrigeri wrote:
> This popped up on my radar when looking at recent changes on the devel branch (which I do before merging to my local devel); I had not looked at it earlier because 1. it’s not my job; 2. I don’t want to be a control freak by default; 3. I had made my expectations clear during an ASP meeting in December or January and bertagaz had said he had taken note of them.

I did!

> I’ve read the diff and commit messages a few times and I don’t understand why we do the “I configure APT to use non-onion sources” dance. It’s questionable to test something different than what users do but I assume there’s a good reason for that. However I am not able to guess what that reason is so could you please add a comment that makes this clear? Thanks in advance!

There’s a comment, but maybe it’s not clear enough:

    # We have to save the non-onion APT sources in persistence, so
    # that on next boot the additional software packages service has
    # the right APT indexes to install the package we want.

As our test suite is using a fake Tor network, we have to replace the APT sources to non-hidden ones in the same way we do for the APT feature. If we want the package to be installed by ASP when we reboot in the test suite, we have to keep the APT indexes for the non-hidden APT sources hosts, otherwise APT gets confused and don’t want to install the package.

Is it clearer? Do you have some rephrasing improvements for the comment?

> Also, it seems to me that the added test only exercises very basic usage of ASP in offline mode, and in particular only one of the 3 failure modes we’ve identified on the blueprint, i.e. “One of the packages was not cached in the first place” that was fixed in 3.5. I see no explanation for not testing the other failure modes, so here we go. This test suite checks neither:
>
> * “APT indices have expired”: I think it’s acceptable because I don’t think “APT checks the indices expiration date only when it downloads them, not when it reads them to install packages” will ever change silently; let’s hope someone will notice the change in APT’s changelog if this ever changes (and I assume there will be a config setting available to switch back to the old behaviour). So in this case I don’t think the cost/benefit of writing a test for that potential failure mode is worth it :)

Yes, that’s what I thought and understood.

> * “Incomplete online upgrade process” (Feature #14570) whose fix was merged at the same time as this branch for 3.6. That’s the only updated piece of code we have for Feature #14570 so as I made clear in our December (or January?) meeting, I would expect us to exercise it. I understand it may be non-trivial to do so. I could live with a conclusion of “the cost/benefit is not worth it” if that’s the case, but I think there needs to be a clear reasoning that leads to that conclusion.

I noted this scenario. But this test is a bit more complex to implement, and too much for the time I had in hands to complete this first step. But I did not forget it: my intend is to introduce this case in the following developments of this feature. I’ve added a comment in Feature #14596#note-7 to make it clear.

#36 Updated by intrigeri 2018-02-21 16:16:09

  • QA Check changed from Info Needed to Dev Needed

Hi!

>> I’ve read the diff and commit messages a few times and I don’t understand why we do the “I configure APT to use non-onion sources” dance. It’s questionable to test something different than what users do but I assume there’s a good reason for that. However I am not able to guess what that reason is so could you please add a comment that makes this clear? Thanks in advance!

> There’s a comment, but maybe it’s not clear enough: […]

> As our test suite is using a fake Tor network, we have to replace the APT sources to non-hidden ones in the same way we do for the APT feature.

Now I get it! So please add a comment above “I configure APT to use non-onion sources” that points to commit:e2510fae79870ff724d190677ff3b228b2bf7eac that has a more complete explanation then (I would not ask for a comment if the commit message that introduced this line made it clear already, but that’s not the case).

>> * “Incomplete online upgrade process” (Feature #14570) whose fix was merged at the same time as this branch for 3.6. That’s the only updated piece of code we have for Feature #14570 so as I made clear in our December (or January?) meeting, I would expect us to exercise it. I understand it may be non-trivial to do so. I could live with a conclusion of “the cost/benefit is not worth it” if that’s the case, but I think there needs to be a clear reasoning that leads to that conclusion.

> I noted this scenario. But this test is a bit more complex to implement, and too much for the time I had in hands to complete this first step.

I see.

> But I did not forget it: my intend is to introduce this case in the following developments of this feature. I’ve added a comment in Feature #14596#note-7 to make it clear.

Thanks. I find it very confusing, in terms of Redmine semantics, to close the “Implement automated tests for Additional Software/Offline Mode” ticket and postpone the biggest part of it to another ticket that’s called “Write automated tests for Additional Software GUI”, but whatever, I won’t be personally affected by this much and I’m not the manager for this team, so I’ll look elsewhere :) But please consider adding this to description of Feature #14596: that’s where we describe what is the expected outcome of a ticket, while comments tend to get lost/forgotten once the discussion on a ticket grows even to medium levels.

#37 Updated by bertagaz 2018-02-21 16:40:46

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

intrigeri wrote:
> Now I get it! So please add a comment above “I configure APT to use non-onion sources” that points to commit:e2510fae79870ff724d190677ff3b228b2bf7eac that has a more complete explanation then (I would not ask for a comment if the commit message that introduced this line made it clear already, but that’s not the case).

Sorry, I thought you knew about this. Worth documenting correctly, for sure. See commit:c02640b43392d51d989decdedb1fd86db8991ee2

> Thanks. I find it very confusing, in terms of Redmine semantics, to close the “Implement automated tests for Additional Software/Offline Mode” ticket and postpone the biggest part of it to another ticket that’s called “Write automated tests for Additional Software GUI”, but whatever, I won’t be personally affected by this much and I’m not the manager for this team, so I’ll look elsewhere :) But please consider adding this to description of Feature #14596: that’s where we describe what is the expected outcome of a ticket, while comments tend to get lost/forgotten once the discussion on a ticket grows even to medium levels.

Done.

#38 Updated by intrigeri 2018-02-21 17:18:28

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

> Sorry, I thought you knew about this.

I did know about this a year ago but I forgot. And regardless: I certainly hope that other people than me will read that file some day and we should not assume they can guess (I mean, I failed to guess while I implemented the same workaround myself a year ago :)

> Worth documenting correctly, for sure. See commit:c02640b43392d51d989decdedb1fd86db8991ee2

LGTM!

Note that “refs: Feature #11556” will mark that ticket as “In progress”. I’m pretty sure that’s not what you meant to do :)

#39 Updated by intrigeri 2018-02-21 17:31:56

  • Assignee deleted (intrigeri)

#40 Updated by bertagaz 2018-02-21 20:40:00

  • Status changed from Fix committed to In Progress

Applied in changeset commit:c02640b43392d51d989decdedb1fd86db8991ee2.

#41 Updated by intrigeri 2018-02-22 09:17:52

  • Status changed from In Progress to Fix committed

bertagaz wrote:
> Applied in changeset commit:c02640b43392d51d989decdedb1fd86db8991ee2.

That commit erroneously caused this ticket to be set back to “In progress”.

#42 Updated by bertagaz 2018-03-14 11:12:34

  • Status changed from Fix committed to Resolved

#43 Updated by intrigeri 2018-08-17 06:54:10

  • related to Bug #15799: "Additional software packages are installed even without network" test always fail in my environment added