Merge pull request #198 from trema/feature/refactor_hello

Refactor Pio::Hello and its tests.

Rakefile

@@ -1,7 +1,7 @@
 require 'bundler/gem_tasks'
 
 RELISH_PROJECT = 'trema/pio'
-FLAY_THRESHOLD = 1539
+FLAY_THRESHOLD = 1467
 
 task default: :travis
 task test: [:spec, :cucumber]

features/open_flow10/echo_reply.feature

@@ -50,26 +50,12 @@ Feature: Pio::Echo::Reply
       | body           |                  |
       | user_data      |                  |
 
-  Scenario: new(xid: -1) and error
-    When I try to create an OpenFlow message with:
-      """
-      Pio::Echo::Reply.new(xid: -1)
-      """
-    Then it should fail with "ArgumentError", "Transaction ID should be an unsigned 32-bit integer."
-
-  Scenario: new(xid: 2**32) and error
-    When I try to create an OpenFlow message with:
-      """
-      Pio::Echo::Reply.new(xid: 2**32)
-      """
-    Then it should fail with "ArgumentError", "Transaction ID should be an unsigned 32-bit integer."
-
   Scenario: new(unknown_attr: 'foo') and error
     When I try to create an OpenFlow message with:
       """
       Pio::Echo::Reply.new(unknown_attr: 'foo')
       """
-    Then it should fail with "RuntimeError", "Unknown keyword: unknown_attr"
+    Then it should fail with "RuntimeError", "Unknown option: unknown_attr"
 
   Scenario: new(body: 'echo reply body')
     When I try to create an OpenFlow message with:

features/open_flow10/echo_request.feature

@@ -50,20 +50,6 @@ Feature: Pio::Echo::Request
       | body           |                    |
       | user_data      |                    |
 
-  Scenario: new(xid: -1) and error
-    When I try to create an OpenFlow message with:
-      """
-      Pio::Echo::Request.new(xid: -1)
-      """
-    Then it should fail with "ArgumentError", "Transaction ID should be an unsigned 32-bit integer."
-
-  Scenario: new(xid: 2**32) and error
-    When I try to create an OpenFlow message with:
-      """
-      Pio::Echo::Request.new(xid: 2**32)
-      """
-    Then it should fail with "ArgumentError", "Transaction ID should be an unsigned 32-bit integer."
-
   Scenario: new(body: 'echo request body')
     When I try to create an OpenFlow message with:
       """

features/open_flow10/hello.feature

@@ -14,6 +14,7 @@ Feature: Pio::Hello
       | transaction_id |          0 |
       | xid            |          0 |
       | body           |            |
+      | user_data      |            |
 
   Scenario: new(transaction_id: 123)
     When I try to create an OpenFlow message with:
@@ -30,7 +31,8 @@ Feature: Pio::Hello
       | transaction_id |        123 |
       | xid            |        123 |
       | body           |            |
-
+      | user_data      |            |
+
   Scenario: new(xid: 123)
     When I try to create an OpenFlow message with:
       """
@@ -46,20 +48,14 @@ Feature: Pio::Hello
       | transaction_id |        123 |
       | xid            |        123 |
       | body           |            |
+      | user_data      |            |
 
-  Scenario: new(xid: -1) and error
-    When I try to create an OpenFlow message with:
-      """
-      Pio::Hello.new(xid: -1)
-      """
-    Then it should fail with "ArgumentError", "Transaction ID should be an unsigned 32-bit integer."
-
-  Scenario: new(xid: 2**32) and error
+  Scenario: new(unknown_attr: 'foo') and error
     When I try to create an OpenFlow message with:
       """
-      Pio::Hello.new(xid: 2**32)
+      Pio::Hello.new(unknown_attr: 'foo')
       """
-    Then it should fail with "ArgumentError", "Transaction ID should be an unsigned 32-bit integer."
+    Then it should fail with "RuntimeError", "Unknown option: unknown_attr"
 
   Scenario: new(body: 'hello')
     When I try to create an OpenFlow message with:
@@ -76,6 +72,7 @@ Feature: Pio::Hello
       | transaction_id |          0 |
       | xid            |          0 |
       | body           |      hello |
+      | user_data      |      hello |
 
   Scenario: read
     When I try to parse a file named "open_flow10/hello.raw" with "Hello" class
@@ -89,6 +86,7 @@ Feature: Pio::Hello
       | transaction_id |         23 |
       | xid            |         23 |
       | body           |            |
+      | user_data      |            |
 
   Scenario: parse error
     When I try to parse a file named "open_flow10/features_request.raw" with "Pio::Hello" class

features/open_flow13/echo_reply.feature

@@ -106,7 +106,7 @@ Feature: Pio::Echo::Reply
       """
       Pio::Echo::Reply.new(unknown_attr: 'foo')
       """
-    Then it should fail with "RuntimeError", "Unknown keyword: unknown_attr"
+    Then it should fail with "RuntimeError", "Unknown option: unknown_attr"
 
   Scenario: read (no message body)
     When I try to parse a file named "open_flow13/echo_reply_no_body.raw" with "Pio::Echo::Reply" class

features/open_flow13/echo_request.feature

@@ -106,7 +106,7 @@ Feature: Pio::Echo::Request
       """
       Pio::Echo::Request.new(unknown_attr: 'foo')
       """
-    Then it should fail with "RuntimeError", "Unknown keyword: unknown_attr"
+    Then it should fail with "RuntimeError", "Unknown option: unknown_attr"
 
   Scenario: read (no message body)
     When I try to parse a file named "open_flow13/echo_request_no_body.raw" with "Pio::Echo::Request" class

features/open_flow13/features_request.feature

@@ -69,7 +69,7 @@ Feature: Pio::Features::Request
       """
       Pio::Features::Request.new(unknown_attr: 'foo')
       """
-    Then it should fail with "RuntimeError", "Unknown keyword: unknown_attr"
+    Then it should fail with "RuntimeError", "Unknown option: unknown_attr"
 
   Scenario: read
     When I try to parse a file named "open_flow13/features_request.raw" with "Pio::Features::Request" class

lib/pio/open_flow/echo.rb

@@ -36,12 +36,13 @@ def self.message_type(message_type)
         end
       end
 
+      user_option :transaction_id
+      user_option :xid
+      user_option :body
+      user_option :user_data
+
       def initialize(user_options = {})
-        unknown_options =
-          user_options.keys - [:transaction_id, :xid, :body, :user_data]
-        unless unknown_options.empty?
-          fail "Unknown keyword: #{unknown_options.first}"
-        end
+        validate_user_options user_options
         header_options = OpenFlowHeader::Options.parse(user_options)
         body_options = user_options[:body] || user_options[:user_data] || ''
         @format = self.class.const_get(:Format).new(header: header_options,

lib/pio/open_flow/message.rb

@@ -19,9 +19,29 @@ def self.read(raw_data)
       end
       # rubocop:enable MethodLength
 
+      def self.user_option(name)
+        unless class_variable_defined?(:@@valid_options)
+          class_variable_set(:@@valid_options, [])
+        end
+        class_variable_set(:@@valid_options,
+                           class_variable_get(:@@valid_options) + [name])
+      end
+
       def method_missing(method, *args, &block)
         @format.__send__ method, *args, &block
       end
+
+      private
+
+      def validate_user_options(user_options)
+        unknown_options = user_options.keys - valid_options
+        return if unknown_options.empty?
+        fail "Unknown option: #{unknown_options.first}"
+      end
+
+      def valid_options
+        self.class.class_variable_get(:@@valid_options)
+      end
     end
   end
 end

lib/pio/open_flow10/hello.rb

@@ -4,15 +4,25 @@
 module Pio
   # OpenFlow 1.0 Hello message
   class Hello < OpenFlow::Message
-    # OpenFlow 1.0 Hello message
+    # OpenFlow 1.0 Hello message format
     class Format < BinData::Record
       extend OpenFlow::Format
 
       header version: 1, message_type: OpenFlow::HELLO
       string :body
+
+      def user_data
+        body
+      end
     end
 
+    user_option :transaction_id
+    user_option :xid
+    user_option :body
+    user_option :user_data
+
     def initialize(user_options = {})
+      validate_user_options user_options
       header_options = OpenFlowHeader::Options.parse(user_options)
       body_options = user_options[:body] || user_options[:user_data] || ''
       @format = Format.new(header: header_options, body: body_options)

lib/pio/open_flow13/features_request.rb

@@ -19,7 +19,7 @@ class Format < BinData::Record
       def initialize(user_attrs = {})
         unknown_attrs = user_attrs.keys - [:transaction_id, :xid]
         unless unknown_attrs.empty?
-          fail "Unknown keyword: #{unknown_attrs.first}"
+          fail "Unknown option: #{unknown_attrs.first}"
         end
         header_options = OpenFlowHeader::Options.parse(user_attrs)
         @format = Format.new(header: header_options)

lib/pio/open_flow13/hello.rb

@@ -2,14 +2,14 @@
 
 # Base module.
 module Pio
-  remove_const :Hello
+  remove_const :Hello if const_defined?(:Hello)
 
   # OpenFlow 1.3 Hello message parser and generator
   class Hello < OpenFlow::Message
+    VERSION_BITMAP = 1
+
     # ofp_hello_elem_header and value
     class Element < BinData::Record
-      VERSION_BITMAP = 1
-
       endian :big
 
       uint16 :element_type
@@ -70,17 +70,16 @@ def version_bitmap
       end
     end
 
-    def initialize(user_attrs = {})
-      unknown_keywords = user_attrs.keys - [:transaction_id, :xid]
-      unless unknown_keywords.empty?
-        fail "Unknown keyword: #{unknown_keywords.first}"
-      end
+    user_option :transaction_id
+    user_option :xid
 
-      header_attrs = OpenFlowHeader::Options.parse(user_attrs)
-      body_attrs = { elements: [{ element_type: 1,
-                                  element_length: 8,
-                                  element_value: 16 }] }
-      @format = Format.new(header: header_attrs, body: body_attrs)
+    def initialize(user_options = {})
+      validate_user_options user_options
+      header_options = OpenFlowHeader::Options.parse(user_options)
+      body_options = { elements: [{ element_type: VERSION_BITMAP,
+                                    element_length: 8,
+                                    element_value: 0b10000 }] }
+      @format = Format.new(header: header_options, body: body_options)
     end
   end
 end

spec/pio/open_flow10/echo_reply_spec.rb

@@ -0,0 +1,7 @@
+require 'pio/open_flow10/echo'
+
+describe Pio::Echo::Reply do
+  describe '.new' do
+    it_should_behave_like('an OpenFlow message', Pio::Echo::Reply)
+  end
+end

spec/pio/open_flow10/echo_request_spec.rb

@@ -0,0 +1,7 @@
+require 'pio/open_flow10/echo'
+
+describe Pio::Echo::Request do
+  describe '.new' do
+    it_should_behave_like('an OpenFlow message', Pio::Echo::Request)
+  end
+end

spec/pio/open_flow10/hello_spec.rb

@@ -0,0 +1,7 @@
+require 'pio/open_flow10/hello'
+
+describe Pio::Hello do
+  describe '.new' do
+    it_should_behave_like('an OpenFlow message', Pio::Hello)
+  end
+end

spec/pio/open_flow13/hello_spec.rb

@@ -1,4 +1,5 @@
 require 'pio/open_flow13/hello'
+require 'pio/parse_error'
 
 describe Pio::Hello do
   describe '.read' do
@@ -38,6 +39,8 @@
   end
 
   describe '.new' do
+    it_should_behave_like('an OpenFlow message', Pio::Hello)
+
     context 'with no arguments' do
       When(:result) { Pio::Hello.new }
 
@@ -53,61 +56,11 @@
       end
     end
 
-    context 'with transaction_id: 123' do
-      When(:result) { Pio::Hello.new(transaction_id: 123) }
-
-      Then { result.ofp_version == 4 }
-      Then { result.message_type == 0 }
-      Then { result.message_length == 16 }
-      Then { result.transaction_id == 123 }
-      Then { result.xid == 123 }
-      Then { result.supported_versions == [:open_flow13] }
-      Then do
-        result.to_binary ==
-          [4, 0, 0, 16, 0, 0, 0, 123, 0, 1, 0, 8, 0, 0, 0, 16].pack('C*')
-      end
-    end
-
-    context 'with xid: 123' do
-      When(:result) { Pio::Hello.new(xid: 123) }
-
-      Then { result.ofp_version == 4 }
-      Then { result.message_type == 0 }
-      Then { result.message_length == 16 }
-      Then { result.transaction_id == 123 }
-      Then { result.xid == 123 }
-      Then { result.supported_versions == [:open_flow13] }
-      Then do
-        result.to_binary ==
-          [4, 0, 0, 16, 0, 0, 0, 123, 0, 1, 0, 8, 0, 0, 0, 16].pack('C*')
-      end
-    end
-
-    context 'with xid: -1' do
-      When(:result) { Pio::Hello.new(xid: -1) }
-
-      Then do
-        result ==
-          Failure(ArgumentError,
-                  'Transaction ID should be an unsigned 32-bit integer.')
-      end
-    end
-
-    context 'with xid: 2**32' do
-      When(:result) { Pio::Hello.new(xid: 2**32) }
-
-      Then do
-        result ==
-          Failure(ArgumentError,
-                  'Transaction ID should be an unsigned 32-bit integer.')
-      end
-    end
-
-    context 'with invalid_keyword: :foo' do
-      When(:result) { Pio::Hello.new(invalid_keyword: :foo) }
+    context 'with invalid_option: :foo' do
+      When(:result) { Pio::Hello.new(invalid_option: :foo) }
 
       Then do
-        result == Failure(RuntimeError, 'Unknown keyword: invalid_keyword')
+        result == Failure(RuntimeError, 'Unknown option: invalid_option')
       end
     end
   end