Project

General

Profile

Feature #14572

Feature #14568: Additional Software Packages

Implement automated tests for Additional Software/Offline Mode

Added by u about 1 year ago. Updated 6 months ago.

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

100%

QA Check:
Pass
Feature Branch:
feature/14572-automated-tests-for-ASP-offline-mode
Type of work:
Code
Starter:
Affected tool:
Additional Software Packages

Description

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


Related issues

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

Associated revisions

Revision 99566229 (diff)
Added by intrigeri 12 months ago

Manual test suite: clarify screen keyboard tests so they are better specified and serve as regression tests (refs: #14572).

Revision 53bdfcdb (diff)
Added by bertagaz 8 months ago

Test suite: add additional software packages feature.

This first iteration implements test of the automatic installation of
additional software packages when Tails is booted in offline mode.

Refs: #14572, #14570, #9059

Revision 7dfa9761
Added by bertagaz 7 months ago

Merge remote-tracking branch 'origin/feature/14572-automated-tests-for-ASP-offline-mode' into devel

Fix-committed: #14570, #14572

Revision c02640b4 (diff)
Added by bertagaz 7 months ago

ASP: reference the commit explaining how to use APT in the test suite.

Using chutney requires to use non-onion APT sources.

Refs: #14572, #11556

Revision 6b13c981 (diff)
Added by intrigeri 6 months ago

Onion Grater: adjust parsing of /proc/pid/attr/current to support named AppArmor profiles (refs: #14572)

Without this changes, passing 'torbrowser_firefox' to the exe-paths setting
is not effective.

Revision 02bee255 (diff)
Added by intrigeri 6 months ago

Test suite: update Tor Browser AppArmor profile name (refs: #14572)

History

#1 Updated by u about 1 year ago

  • Parent task set to #14568

#2 Updated by u about 1 year ago

  • Due date set to 01/15/2018

#3 Updated by u about 1 year ago

  • Description updated (diff)

#4 Updated by u about 1 year ago

  • Description updated (diff)

#6 Updated by u about 1 year ago

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

#7 Updated by u about 1 year ago

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

#8 Updated by u about 1 year ago

#9 Updated by u about 1 year ago

  • Type of work changed from Sysadmin to Code

#10 Updated by intrigeri about 1 year ago

#11 Updated by u about 1 year ago

  • Affected tool set to Additional Software Packages

#12 Updated by sajolida about 1 year ago

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

#13 Updated by u about 1 year ago

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

thanks, sajolida!

#14 Updated by intrigeri 12 months ago

  • Status changed from Confirmed to In Progress

#15 Updated by intrigeri 9 months ago

  • 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 9 months ago

The only remaining TBD item on the code side (#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 9 months ago

  • Status changed from In Progress to Confirmed

#18 Updated by u 9 months ago

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 u 8 months ago

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 8 months ago

  • 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 #9059 with #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 8 months ago

  • Status changed from Confirmed to In Progress

#22 Updated by u 8 months ago

bertagaz wrote:

Applied in changeset 53bdfcdb7d0f40f021a94fb0b7dbfcf9ab5f6761.

Thanks!

#23 Updated by intrigeri 8 months ago

branch which is based on the one from #9059 with #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 8 months ago

intrigeri wrote:

branch which is based on the one from #9059 with #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 8 months ago

intrigeri wrote:

branch which is based on the one from #9059 with #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 8 months ago

branch which is based on the one from #9059 with #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 #14570 is relevant here, but given how deeply the integration is being reworked on #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 8 months ago

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

#28 Updated by anonym 8 months ago

  • 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 u 8 months ago

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

#30 Updated by intrigeri 8 months ago

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

#31 Updated by intrigeri 8 months ago

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

#32 Updated by bertagaz 7 months ago

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

#33 Updated by bertagaz 7 months ago

  • 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 7 months ago

  • 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" (#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 #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 7 months ago

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" (#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 #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 #14596#note-7 to make it clear.

#36 Updated by intrigeri 7 months ago

  • 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 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" (#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 #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 #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 #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 7 months ago

  • 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 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 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 #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 7 months ago

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

LGTM!

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

#39 Updated by intrigeri 7 months ago

  • Assignee deleted (intrigeri)

#40 Updated by bertagaz 7 months ago

  • Status changed from Fix committed to In Progress

#41 Updated by intrigeri 7 months ago

  • Status changed from In Progress to Fix committed

bertagaz wrote:

Applied in changeset c02640b43392d51d989decdedb1fd86db8991ee2.

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

#42 Updated by bertagaz 6 months ago

  • Status changed from Fix committed to Resolved

#43 Updated by intrigeri about 1 month ago

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

Also available in: Atom PDF