Re: [Tails-dev] Please review'n'merge test/firewall-check-ta…

Delete this message

Reply to this message
Author: intrigeri
Date:  
To: The Tails public development discussion list
Subject: Re: [Tails-dev] Please review'n'merge test/firewall-check-tag [was: Please review'n'merge test/reorg]
hi,

anonym wrote (06 Aug 2013 13:58:23 GMT) :
> Hopefully fixed in commit 942a030, together with the previous
> encapsulation issue in test/reorg.


Great!

>>> def assert_no_leaks
>>
>> Now that this isn't a step definition anymore, but an instance method,
>> I don't think `puts' is the optimal way to report detailed information
>> before throwing an exception. How about storing the detailed text in
>> a local variable, in each of the `if' blocks, and then passing this
>> additional information to `raise'?


> Fixed in commit 894f782.


>    def save_pcap_file
>      pcap_copy = "#{@pcap_file}-#{DateTime.now}"
>      FileUtils.cp(@pcap_file, pcap_copy)
> -    puts "Full network capture available at: #{pcap_copy}"
> +    pcap_copy
>    end


It seems that this now returns a meaningful value, which is used later
on, so perhaps document the function's return value as something that
can be relied upon for other uses?

Looks good to me otherwise :)

>> It seems to me 48a32dd introduces a dependency on @custom_sniffer,
>> while this attribute is only created by the next commit.
>> This breaks bisection.


> Ah, right. I could squash those commits when you think this branch is
> merge-ready.


This will be great, yes.

Cheers!
--
intrigeri
| GnuPG key @ https://gaffer.ptitcanardnoir.org/intrigeri/intrigeri.asc
| OTR fingerprint @ https://gaffer.ptitcanardnoir.org/intrigeri/otr.asc