Tails

#1 Updated by bertagaz 2013-09-26 05:44:01

• Subject changed from Macchanger test to Write macchanger tests

#2 Updated by intrigeri 2013-09-26 06:14:24

• Target version set to Sustainability_M1

#3 Updated by intrigeri 2014-07-10 15:33:17

• Subject changed from Write macchanger tests to Write MAC spoofing tests

#4 Updated by intrigeri 2014-07-10 15:33:57

• related to Feature #7546: Add MAC spoofing to the manual test suite added

#5 Updated by intrigeri 2015-01-06 12:59:39

• blocks #8538 added

#6 Updated by anonym 2015-01-09 14:46:43

• Target version changed from Sustainability_M1 to Tails_1.8

#8 Updated by anonym 2015-01-10 17:52:56

• Assignee set to kytv
• Target version changed from Tails_1.8 to Tails_1.6

#9 Updated by anonym 2015-03-03 20:33:52

• Status changed from Confirmed to In Progress
• Assignee changed from kytv to anonym

I did some work on this, which I’ll try to rebase and split up to atomic commits.

#10 Updated by anonym 2015-03-04 18:28:16

• Feature Branch set to test/6302-mac-spoofing

anonym wrote:
> I did some work on this, which I’ll try to rebase and split up to atomic commits.

Done. It definitely needs some more work, though.

#13 Updated by anonym 2015-09-07 09:31:10

• blocked by Bug #10160: MAC spoofing panic mode is broken added

#14 Updated by anonym 2015-09-07 09:37:39

• Assignee changed from anonym to kytv
• % Done changed from 0 to 50
• QA Check set to Ready for QA

When running the test suite with this branch checked out (i.e. testing it :)) it’s important to build the image from the same branch (which we generally require any way). I also threw in some drive-by refactoring/cleanup work that affects a large part of the test suite but it’s mostly mechanic change which shouldn’t be hard to review, and just require a full run to test; that’s commit commit:1439180. I hope that’s not an issue.

kytv, when reviewing, it would be great if you also had a look at Bug #10160, whose fix this branch includes by necessity. Also, you may be interested in having a look at robust_notification_wait() from commit commit:113dbf7. Perhaps you’ve seen other places which may benefit from that more robust implementation (all places?).

#19 Updated by kytv 2015-09-17 08:57:38

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

After the code review and testing I’m happy with this branch other than the minor typo mentioned at Feature #6302#note-18.

Re-assigning to anonym to fix the typo before this gets sent to intrigeri.

#20 Updated by anonym 2015-09-17 16:41:58

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

kytv wrote:
> After the code review and testing I’m happy with this branch other than the minor typo mentioned at Feature #6302#note-18.

Fixed, thanks!

> Re-assigning to anonym to fix the typo before this gets sent to intrigeri.

Did it for you.

intigeri, I’m quite sure you’ll have time with this, so I guess we should postpone to 1.7, right?

#21 Updated by intrigeri 2015-09-22 01:33:18

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

#22 Updated by intrigeri 2015-09-22 02:26:50

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

Hi, FYI this branch doesn’t merge cleanly into current stable.

#23 Updated by intrigeri 2015-09-22 06:47:37

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

#24 Updated by intrigeri 2015-09-22 07:32:02

• Assignee changed from intrigeri to anonym
• % Done changed from 50 to 60
• QA Check changed from Ready for QA to Dev Needed

Great work. I’ll nitpick quite a bit on code style below, since that’s a biggish set of changes, and as usual I’ve put myself in the “what if I have to understand how this thing works to fix a serious bug in there, in a hurry, some day” mindset for reviewing ;)

• mac_spoofing.feature lacks a description (“As a […]”)
• I’ve found it very confusing to see “And no network devices are present” and later “Then 1 network device is present”. Initially I understood that “present” = “plugged in the computer”, so I was looking for understand how a network device had appeared. May you please improve the clarity of this aspect a bit?
• It strikes me as odd to read “I simulate that” in Gherkin text: we’re simulating a *load of stuff anyway and IMO part of the point is to hide this fact at the Gherkin abstraction level. Let’s just describe the reality we want when we’ll be using mocks and stuff anyway.
• I really like the way you’ve improved code readability with the fancy new libs => thing :) More sugar!
• “And disable MAC spoofing in Tails Greeter” needs a subject (“I”): the way it’s written right now relies on a specific ordering between steps, which is no good cucumber style I think.
• In for addr_type in ["nic_ipv4_addr", "nic_ipv6_addr"] do, it seems that the addr_type variable could be better named, no?
• “The below log was recorded from Tails based on Debian Wheezy” wants a ticket for Jessie, blocked by this one. Perhaps the code comment could be more general, to express the fact that installing a new NM (say, from backports) requires the same update process. Not sure where we can list such checks we have to do on a regular basis, btw.
• Reading how we use the mac_leaks variable I wonder why it’s not a set.
• I don’t understand why host_real_mac is called this way.
• Reading the next few lines, in libs = [libs] if libs.class != Array it seems like we could test only for having a “map” method if we want the code to be more flexible (and thus easier to change in the future).
• I’ve pushed a fix for a typo in a code comment (showed up in the diff context).

This change seems to have been introduced as part of a conflict resolution today:

-       "xdotool set_desktop '#{desktop_number}'", user
+      "xdotool search --name '#{desktop_number}' windowactivate --sync",

… and I’m not sure it’s correct. May you please double-check? Especially, def do_focus below uses execute_successfully diffently so I wonder if the conflict resolution was good. Not run any tests yet, though.

#25 Updated by anonym 2015-10-04 02:50:24

• blocks Bug #10336: Update "blocked wireless device" trace for Jessie added

#26 Updated by anonym 2015-10-04 02:55:23

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

intrigeri wrote:
> Great work. I’ll nitpick quite a bit on code style below, since that’s a biggish set of changes, and as usual I’ve put myself in the “what if I have to understand how this thing works to fix a serious bug in there, in a hurry, some day” mindset for reviewing ;)

And I’m thankful for it! :)

> * mac_spoofing.feature lacks a description (“As a […]”)

commit:1795261

> * I’ve found it very confusing to see “And no network devices are present” and later “Then 1 network device is present”. Initially I understood that “present” = “plugged in the computer”, so I was looking for understand how a network device had appeared. May you please improve the clarity of this aspect a bit?

Indeed, “device” is ambiguous. What about commit:1017550?

> * It strikes me as odd to read “I simulate that” in Gherkin text: we’re simulating a *load of stuff anyway and IMO part of the point is to hide this fact at the Gherkin abstraction level. Let’s just describe the reality we want when we’ll be using mocks and stuff anyway.

Yeah, sure. :) commit:afb3521

> * I really like the way you’ve improved code readability with the fancy new libs => thing :) More sugar!

Yay! For the record, I already talked about this with kytv, but if you ever feel like you want to add methods with optional parameters, please use the Hash trick instead. It has a lot of advantages.

> * “And disable MAC spoofing in Tails Greeter” needs a subject (“I”): the way it’s written right now relies on a specific ordering between steps, which is no good cucumber style I think.

commit:9be839e

> * In for addr_type in ["nic_ipv4_addr", "nic_ipv6_addr"] do, it seems that the addr_type variable could be better named, no?

commit:29668da

> * “The below log was recorded from Tails based on Debian Wheezy” wants a ticket for Jessie, blocked by this one. Perhaps the code comment could be more general, to express the fact that installing a new NM (say, from backports) requires the same update process.

#27 Updated by anonym 2015-10-04 02:55:49

• blocked by Feature #6094: Test suite: background snapshot improvements added

#28 Updated by intrigeri 2015-10-05 02:30:18

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

I’m not commenting on the changes that entirely fix my concerns. Thanks for doing them, great work! There’s only one code blocker left below, and then I’ll run the tests.

>> Not sure where we can list such checks we have to do on a regular basis, btw.

> No idea. :/

Let’s just s/XXX: Jessie/XXX: Stretch/ in code comments for the time being.
When I port Tails to a new Debian I generally git grep $next_codename.

>> * Reading how we use the mac_leaks variable I wonder why it’s not a set.

> commit:3e6c30a

Great, thanks.

No big deal, not a blocker, but: I’m no fan of storing temporary state still under construction in an instance variable, which this commit silently starts doing, though; sounds like suboptimal OOP design to me.

>> * I don’t understand why host_real_mac is called this way.

> I’m not entirely sure what you mean, but perhaps it is fixed by commit:fb826a4?

Sorry, I meant “named this way”, more accurately than “called this way”. Let me clarify: in the context of code that’s about managing VMs, “host” implicitly refers to the host system, and then if I read host_real_mac somewhere I think it’s about the host system’s MAC. It’s wrong, isn’t it?

#29 Updated by anonym 2015-10-06 03:35:55

• blocks Feature #10340: Automatically test the Greeter's Disable All Networking option added

#30 Updated by anonym 2015-10-06 04:04:37

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

intrigeri wrote:
> I’m not commenting on the changes that entirely fix my concerns. Thanks for doing them, great work! There’s only one code blocker left below, and then I’ll run the tests.

Run, run, run!

Actually I’m first assigning it to kytv for a test run (and a review of the commits pushed since his last look, in case intrigeri missed something; nitpicking time, kytv!). kytv, please reassign to intrigeri once you’re done. With my RM hat on: preferably do this within a week since this ticket is blocking other tickets targeting 1.7.
> >> Not sure where we can list such checks we have to do on a regular basis, btw.
>
> > No idea. :/
>
> Let’s just s/XXX: Jessie/XXX: Stretch/ in code comments for the time being.
> When I port Tails to a new Debian I generally git grep $next_codename.

I think we have to to git grep -iE $current_codename|$next_codename; we sometimes have XXX: workaround for $current version. Then we’ll find the current comment.

> >> * Reading how we use the mac_leaks variable I wonder why it’s not a set.
>
> > commit:3e6c30a
>
> Great, thanks.
>
> No big deal, not a blocker, but: I’m no fan of storing temporary state still under construction in an instance variable, which this commit silently starts doing, though; sounds like suboptimal OOP design to me.

Sure, but when it’s done inside a constructor I think it’s fine. I guess it’s best to be consistent, though: commit:7da257c

> >> * I don’t understand why host_real_mac is called this way.
>
> > I’m not entirely sure what you mean, but perhaps it is fixed by commit:fb826a4?
>
> Sorry, I meant “named this way”, more accurately than “called this way”. Let me clarify: in the context of code that’s about managing VMs, “host” implicitly refers to the host system, and then if I read host_real_mac somewhere I think it’s about the host system’s MAC. It’s wrong, isn’t it?

Duh! You are completely right. Fixed in commit:507c93e.

#33 Updated by kytv 2015-10-21 10:23:24

• Assignee changed from kytv to intrigeri

Sorry for the delay. I’m happy with the branch.

#35 Updated by intrigeri 2015-10-23 10:01:10

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

Applied in changeset commit:dfa1d43d1a422e256c9a4222a187b7fbaa932393.

#36 Updated by intrigeri 2015-10-23 10:02:28

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

Congrats :)

#37 Updated by anonym 2015-11-03 11:33:30

• Status changed from Fix committed to Resolved