New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added be_routed matcher #92

Merged
merged 1 commit into from May 14, 2013

Conversation

Projects
None yet
2 participants
@studio3104
Contributor

studio3104 commented May 7, 2013

Hi. I have added the matcher of routing table.

examples

describe '33.33.33.0' do
  it { should be_routed }
end
describe '33.33.33.0' do
  it { should be_routed.with( :iface => 'eth1' ) }
end
describe '33.33.33.0' do
  it { should be_routed.with(
    :iface => 'eth1',
    :gateway => '0.0.0.0',
    :flags => 'U',
    :genmask => '255.255.255.0'
  ) }
end

Thank you confirmation.

@mizzy

This comment has been minimized.

Show comment
Hide comment
@mizzy

mizzy May 7, 2013

Owner

Man page of route says "This program is obsolete. For replacement check ip route."

So I don't like to use route command.

And the command ip route brings the output like this.

$ ip route
192.168.10.0/24 dev br0  proto kernel  scope link  src 192.168.10.11 
192.168.122.0/24 dev virbr0  proto kernel  scope link  src 192.168.122.1 
169.254.0.0/16 dev br0  scope link  metric 1004 
default via 192.168.10.1 dev br0 

This is very differ from route command.

So you should write a matcher independent from the format of route command and ip route command.

Owner

mizzy commented May 7, 2013

Man page of route says "This program is obsolete. For replacement check ip route."

So I don't like to use route command.

And the command ip route brings the output like this.

$ ip route
192.168.10.0/24 dev br0  proto kernel  scope link  src 192.168.10.11 
192.168.122.0/24 dev virbr0  proto kernel  scope link  src 192.168.122.1 
169.254.0.0/16 dev br0  scope link  metric 1004 
default via 192.168.10.1 dev br0 

This is very differ from route command.

So you should write a matcher independent from the format of route command and ip route command.

@studio3104

This comment has been minimized.

Show comment
Hide comment
@studio3104

studio3104 May 8, 2013

Contributor

Thank you for advice.
I stopped to use netstat command, use ip command.

And then, I chose the format that is independent from the command of either.

example for test of routing table.

describe '33.33.33.0' do
  it { should be_routed }
  it do
    should be_routed.with(
      :interface => 'eth1',
      :gateway => 'default',
      :netmask => '24'
    )
  end
end

example for test of default gateway's ip address.

describe 'default' do
  it { should be_routed }
  it do
    should be_routed.with(
      :interface => 'eth0',
      :gateway => '10.0.2.2',
    )
  end
end

Thank you confirmation.

Contributor

studio3104 commented May 8, 2013

Thank you for advice.
I stopped to use netstat command, use ip command.

And then, I chose the format that is independent from the command of either.

example for test of routing table.

describe '33.33.33.0' do
  it { should be_routed }
  it do
    should be_routed.with(
      :interface => 'eth1',
      :gateway => 'default',
      :netmask => '24'
    )
  end
end

example for test of default gateway's ip address.

describe 'default' do
  it { should be_routed }
  it do
    should be_routed.with(
      :interface => 'eth0',
      :gateway => '10.0.2.2',
    )
  end
end

Thank you confirmation.

@studio3104

This comment has been minimized.

Show comment
Hide comment
@studio3104

studio3104 May 9, 2013

Contributor

By the way, about the name of the matcher.
I came up with only these...

  • be_routable
  • have_routing

How do you think??

Contributor

studio3104 commented May 9, 2013

By the way, about the name of the matcher.
I came up with only these...

  • be_routable
  • have_routing

How do you think??

@mizzy

This comment has been minimized.

Show comment
Hide comment
@mizzy

mizzy May 9, 2013

Owner

I think have_routing is better.

Owner

mizzy commented May 9, 2013

I think have_routing is better.

@mizzy

This comment has been minimized.

Show comment
Hide comment
@mizzy

mizzy May 9, 2013

Owner

How about this?

describe 'routing table' do
  it { should have_routing_entry( :destination => 'default', :gateway => '192.168.0.1' )
end
Owner

mizzy commented May 9, 2013

How about this?

describe 'routing table' do
  it { should have_routing_entry( :destination => 'default', :gateway => '192.168.0.1' )
end
@studio3104

This comment has been minimized.

Show comment
Hide comment
@studio3104

studio3104 May 9, 2013

Contributor

cool!!
I will try to fix as you said!!!!

Contributor

studio3104 commented May 9, 2013

cool!!
I will try to fix as you said!!!!

@mizzy

This comment has been minimized.

Show comment
Hide comment
@mizzy

mizzy May 9, 2013

Owner

Above example contains 'routing' twice, so it would be better like this.

describe 'routing table' do
  it { should have_entry( :destination => 'default', :gateway => '192.168.0.1' )
end
Owner

mizzy commented May 9, 2013

Above example contains 'routing' twice, so it would be better like this.

describe 'routing table' do
  it { should have_entry( :destination => 'default', :gateway => '192.168.0.1' )
end
@studio3104

This comment has been minimized.

Show comment
Hide comment
@studio3104

studio3104 May 9, 2013

Contributor

Just leave have_entry, I feel that there are too versatile to...

'iptables' is used twice in example of have_iptables_rule, is it fine?

Contributor

studio3104 commented May 9, 2013

Just leave have_entry, I feel that there are too versatile to...

'iptables' is used twice in example of have_iptables_rule, is it fine?

@mizzy

This comment has been minimized.

Show comment
Hide comment
@mizzy

mizzy May 9, 2013

Owner

When have_iptables_rule was implemented, I think I can't recognize what rule it is with only have_rule.

But I realized that I could recognize what rule it is by checking a subject.

If two types of subjects both use have_entry matcher, you can write code like this.

RSpec::Matchers.define :have_entry do |attr|
  match do |subject|
    if subject == 'routing table'
      backend.check_routing_table(example, attr)
    elsif subject = 'foo'
      backend.check_foo(example, attr)
    end
  end
end

And specs are like these.

describe 'iptable' do
  it { should have_entry ... }
end

describe 'foo' do
  it { should have_entry ... }
end
Owner

mizzy commented May 9, 2013

When have_iptables_rule was implemented, I think I can't recognize what rule it is with only have_rule.

But I realized that I could recognize what rule it is by checking a subject.

If two types of subjects both use have_entry matcher, you can write code like this.

RSpec::Matchers.define :have_entry do |attr|
  match do |subject|
    if subject == 'routing table'
      backend.check_routing_table(example, attr)
    elsif subject = 'foo'
      backend.check_foo(example, attr)
    end
  end
end

And specs are like these.

describe 'iptable' do
  it { should have_entry ... }
end

describe 'foo' do
  it { should have_entry ... }
end
@studio3104

This comment has been minimized.

Show comment
Hide comment
@studio3104

studio3104 May 9, 2013

Contributor

I see!
It is a great idea! !

I understand and accept, and implement that way.

Contributor

studio3104 commented May 9, 2013

I see!
It is a great idea! !

I understand and accept, and implement that way.

@studio3104

This comment has been minimized.

Show comment
Hide comment
@studio3104

studio3104 May 9, 2013

Contributor

I tried to implement as your advice.
confirmation please.

specs example
describe 'routing table' do
  it do
    should have_entry(
      :destination => '33.33.33.0',
      :interface => 'eth1',
      :gateway => 'default',
    )
  end

  it do
    should have_entry(
      :destination => 'default',
      :interface => 'eth0',
      :gateway => '10.0.2.2',
    )
  end
end
Contributor

studio3104 commented May 9, 2013

I tried to implement as your advice.
confirmation please.

specs example
describe 'routing table' do
  it do
    should have_entry(
      :destination => '33.33.33.0',
      :interface => 'eth1',
      :gateway => 'default',
    )
  end

  it do
    should have_entry(
      :destination => 'default',
      :interface => 'eth0',
      :gateway => '10.0.2.2',
    )
  end
end
@mizzy

This comment has been minimized.

Show comment
Hide comment
@mizzy

mizzy May 9, 2013

Owner

`if subject == 'routing table' ... end`` is not needed now.You should add if block when other type of subject uses this matcher.

Owner

mizzy commented May 9, 2013

`if subject == 'routing table' ... end`` is not needed now.You should add if block when other type of subject uses this matcher.

@studio3104

This comment has been minimized.

Show comment
Hide comment
@studio3104

studio3104 May 10, 2013

Contributor

Sorry, I fixed.
Please confirmation again.

Contributor

studio3104 commented May 10, 2013

Sorry, I fixed.
Please confirmation again.

@mizzy

This comment has been minimized.

Show comment
Hide comment
@mizzy

mizzy May 10, 2013

Owner

I'd like to write spec like this.

it do
    should have_entry(
      :destination => '192.168.100.0/24',
      :interface => 'eth0',
      :gateway => '192.168.10.1',
    )
end

And, I don't like you to use "33.33.33.0" as sample IP address.
Please use IP address ranges for private networks.

Owner

mizzy commented May 10, 2013

I'd like to write spec like this.

it do
    should have_entry(
      :destination => '192.168.100.0/24',
      :interface => 'eth0',
      :gateway => '192.168.10.1',
    )
end

And, I don't like you to use "33.33.33.0" as sample IP address.
Please use IP address ranges for private networks.

@studio3104

This comment has been minimized.

Show comment
Hide comment
@studio3104

studio3104 May 10, 2013

Contributor

Sure. You're right. I agree.
I will try to reimplement.

Contributor

studio3104 commented May 10, 2013

Sure. You're right. I agree.
I will try to reimplement.

@studio3104

This comment has been minimized.

Show comment
Hide comment
@studio3104

studio3104 May 13, 2013

Contributor

I reimplement as your advice.

describe 'routing table' do
  it do
    should have_entry(
      :destination => '192.168.100.0/24',
      :interface   => 'eth1',
      :gateway     => '192.168.10.1',
    )
  end
end

And I also added a matcher for the default gateway.
Writing both are possible, but I think the latter is more suitable.

describe 'routing table' do
  it do
    should have_entry(
      :destination => 'default',
      :interface   => 'eth0',
      :gateway     => '192.168.10.1',
    )
  end
end

or

describe '192.168.10.1' do
  it { should be_default_gw }
  it { should be_default_gw.with_interface('eth0') }
end

Please confirmation again.

Contributor

studio3104 commented May 13, 2013

I reimplement as your advice.

describe 'routing table' do
  it do
    should have_entry(
      :destination => '192.168.100.0/24',
      :interface   => 'eth1',
      :gateway     => '192.168.10.1',
    )
  end
end

And I also added a matcher for the default gateway.
Writing both are possible, but I think the latter is more suitable.

describe 'routing table' do
  it do
    should have_entry(
      :destination => 'default',
      :interface   => 'eth0',
      :gateway     => '192.168.10.1',
    )
  end
end

or

describe '192.168.10.1' do
  it { should be_default_gw }
  it { should be_default_gw.with_interface('eth0') }
end

Please confirmation again.

@mizzy

This comment has been minimized.

Show comment
Hide comment
@mizzy

mizzy May 13, 2013

Owner

I like be_default_gateway more than be_default_gw.Could you fix it please?

Owner

mizzy commented May 13, 2013

I like be_default_gateway more than be_default_gw.Could you fix it please?

@studio3104

This comment has been minimized.

Show comment
Hide comment
@studio3104

studio3104 May 13, 2013

Contributor

with_interface is OK??

Contributor

studio3104 commented May 13, 2013

with_interface is OK??

@mizzy

This comment has been minimized.

Show comment
Hide comment
@mizzy

mizzy May 13, 2013

Owner

OK

Sent from Mailbox for iPhone

On Mon, May 13, 2013 at 7:55 PM, studio3104 notifications@github.com
wrote:

with_interface is OK??

Reply to this email directly or view it on GitHub:
#92 (comment)

Owner

mizzy commented May 13, 2013

OK

Sent from Mailbox for iPhone

On Mon, May 13, 2013 at 7:55 PM, studio3104 notifications@github.com
wrote:

with_interface is OK??

Reply to this email directly or view it on GitHub:
#92 (comment)

@studio3104

This comment has been minimized.

Show comment
Hide comment
@studio3104

studio3104 May 13, 2013

Contributor

I fixed, please confirmation again.

Contributor

studio3104 commented May 13, 2013

I fixed, please confirmation again.

@mizzy

This comment has been minimized.

Show comment
Hide comment
@mizzy

mizzy May 13, 2013

Owner

Please fix spec/darwin/commands_spec.rb and matchers_spec.rb.

Owner

mizzy commented May 13, 2013

Please fix spec/darwin/commands_spec.rb and matchers_spec.rb.

@studio3104

This comment has been minimized.

Show comment
Hide comment
@studio3104

studio3104 May 13, 2013

Contributor

Fixed. How about?

Contributor

studio3104 commented May 13, 2013

Fixed. How about?

@mizzy

This comment has been minimized.

Show comment
Hide comment
@mizzy

mizzy May 14, 2013

Owner

LGTM.Thanks!

Owner

mizzy commented May 14, 2013

LGTM.Thanks!

mizzy added a commit that referenced this pull request May 14, 2013

@mizzy mizzy merged commit 488c7ba into mizzy:master May 14, 2013

1 check passed

default The Travis CI build passed
Details
@studio3104

This comment has been minimized.

Show comment
Hide comment
@studio3104

studio3104 May 14, 2013

Contributor

I am glad to merged!!
Thank you so much!!!!

Contributor

studio3104 commented May 14, 2013

I am glad to merged!!
Thank you so much!!!!

@mizzy

This comment has been minimized.

Show comment
Hide comment
@mizzy

mizzy May 14, 2013

Owner

👍

Owner

mizzy commented May 14, 2013

👍

@studio3104 studio3104 deleted the studio3104:add_be_route_matcher branch May 14, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment