27/07/13 20:15, intrigeri wrote:
> anonym wrote (26 Jul 2013 12:49:36 GMT) :
>> Hi,
>
>> branch: test/firewall-check-tag
>> ticket: Feature #5644 <https://labs.riseup.net/code/issues/5644>
>
> This:
>> @mac = REXML::Document.new(@net.xml_desc).elements['network/ip/dhcp/host/'].attributes['mac']
>
> in the Sniffer class, looks like broken encapsulation (see my review
> of test/reorg for details + a proposal about a similar situation).
>
>> - @bridge_name = bridge_name
>> - @mac = mac
>> + @vmnet = vmnet
>> + @net = @vmnet.net
>> + @bridge_name = @net.bridge_name
>> + @mac = REXML::Document.new(@net.xml_desc).elements['network/ip/dhcp/host/'].attributes['mac']
>
> It's not clear to me why @vmnet and @net are added as an instance
> attributes, since they are apparently not used anywhere else in the
> class or by its users. Perhaps it's not worth making the class
> interface & amount of maintained state larger, if all we need in
> practice is a way for some methods to get the MAC address and
> bridge name?
Hopefully fixed in commit 942a030, together with the previous
encapsulation issue in test/reorg.
>> 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.
> 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.
Cheers!
