Permalink
Browse files

Add specs for Machete DSL

  • Loading branch information...
1 parent f149ed8 commit 543e485ac875604be07be0156b8416ef4ab059ab @LTe committed May 30, 2012
Showing with 161 additions and 0 deletions.
  1. +82 −0 spec/machete/dsl/dsl_spec.rb
  2. +79 −0 spec/machete/dsl/to_m_spec.rb
@@ -0,0 +1,82 @@
+require "spec_helper"
+
+module Machete::DSL
+ describe Builder do
+ before do
+ @builder = Builder.new
+ end
+
+ it "should respond to top level method" do
@dmajda

dmajda May 30, 2012

This is probably a matter of personal style, but I try to avoid "should" in the it descriptions. If you are consistent, you start to use it everywhere and it becomes a bit redundant (which is probably why Shoulda has should method instead of it).

I whould write this as it "responds to top level method" and modify the other descriptions in a similar way.

+ @builder.should respond_to :send_with_arguments
+ end
+
+ context "build simple hash" do
+ before do
+ @result = Builder.build do
+ send_with_arguments {}
+ end
+ end
+
+ it "should return proper key in hash" do
+ @result.should include(:SendWithArguments)
+ end
+
+ it "should have empty key" do
+ @result[:SendWithArguments].should == {}
+ end
+ end
@dmajda

dmajda May 30, 2012

I think splitting the check of the key and value is a bit artificial. I'd write this as

it "builds a simple hash" do
  result = Builder.build do
    send_with_arguments {}
  end
  result.should == { :SendWithArguments => {} } 
end

Also, do you have some specific reason to test just the existence of some key in the hash instead of the whole hash (as I did in my example)? I'd say that the method contract is that it returns hash of an exact form (a specific keys and values) and that's what should be tested.

+
+ context "build hash with arguments" do
+ before do
+ @result = Builder.build do
+ send_with_arguments do
+ attribute_1(value: 1)
+ attribute_2(value: 2)
+ array(:array) do
+ attribute_2(value: 2)
+ attribute_3(value: 3)
+ end
+ attribute_4 do
+ nested_attribute(value: "nested")
+ end
+ end
+ end
@dmajda

dmajda May 30, 2012

I'd split this example into one for each it. It unnecessary ties the tests together.

(This is not that big problem here when there are 4 small tests. But imagine you'll add a few more and after a while you won't know which part is there because of what tests. And it could easily happen that you get into such a state that almost any change the example because of one test will break some other ones. It's best to prevent this from the beginning.)

+ @attributes = @result[:SendWithArguments]
+ end
+
+ it "should work with blocks" do
+ @attributes.should include(:attribute_1 => {value: 1})
@dmajda

dmajda May 30, 2012

Same note as above with the inclusion vs. whole hash test (note that more specialized example would make whole hash tests easier).

@dmajda

dmajda May 30, 2012

Please use spaces inside { and } for hash literals (SUSE coding conventions).

It would be also good to stick to one hash syntax for hashes containing symbols (either the 1.8 or 1.9 one) and not combine them (especially not on one line!). I don't care much which of the syntaxes you'll use.

(This applies to other places in this pull request too.)

+ end
+
+ it "should work with hash" do
+ @attributes.should include(:attribute_2 => {value: 2})
+ end
+
+ it "should work with many attributes with the same name (arrays)" do
+ @attributes.should include(:array => [{:attribute_2 => {value: 2}}, {:attribute_3 => {value: 3}}])
+ end
+
+ it "should work with nested attributes" do
+ @attributes[:attribute_4][:nested_attribute].should == {:value => "nested"}
+ end
+ end
+
+ context "reserved words" do
+ before do
+ @result = Builder.build do
+ _send do
+ attribute_1(value: true)
+ end
+ end
+ end
+
+ it "should respond to special methods" do
+ @builder.should respond_to(:_super)
+ end
+
+ it "should build proper hash with special method" do
+ @result.should include(:Send => { :attribute_1 => { value: true } })
+ end
+ end
+ end
+end
@@ -0,0 +1,79 @@
+require "spec_helper"
+
+describe "to_m" do
+ context String do
@dmajda

dmajda May 30, 2012

Just a note: I'd partition the tests differently:

context String do
  describe "to_m" do
     ...
  end
end

And I would put them to files corresponding to the implementation files. But I am OK with your way too.

+ it "should return empty string" do
+ "".to_m.should == ""
+ end
+
+ it "should return 'self'" do
+ "string".to_m.should == "string"
+ end
@dmajda

dmajda May 30, 2012

If you just read the descriptions (e.g. when running the specs in verbose mode), they sound contradictory:

[...] should return empty string
[...] should return 'self'

What about changing them to "returns empty string for an empty string" and "returns itself for a non-empty string" or something similar?

+ end
+
+ context Hash do
+ it "should return empty string in case empty hash" do
@dmajda

dmajda May 30, 2012

Grammar: s/in case/in case of/ (alternatively s/in case/for an/).

+ {}.to_m.should == ""
+ end
+
+ it "should close content with '<>' when key start with capital letter" do
+ {:SendWithArguments => nil}.to_m.should == "SendWithArguments<>"
+ end
+
+ it "should add attribute to AST node" do
+ {:SendWithArguments => { :attribute => 1}}.to_m.should == "SendWithArguments<attribute = 1>"
+ end
+
+ it "should add many attributes to AST node separated by comma" do
+ {
+ :SendWithArguments =>
+ {
+ :attribute_1 => 1,
+ :attribute_2 => 2
+ }
+ }.to_m.should == "SendWithArguments<attribute_1 = 1, attribute_2 = 2>"
+ end
+ end
+
+ context "DSL" do
+ it "should add many attributes with the same name" do
+ dsl = Machete::DSL::Builder.build do
+ send_with_arguments do
+ body(:array) do
+ fixnum_literal(value: 1)
+ fixnum_literal(value: 2)
+ end
+ end
+ end
+
+ dsl.to_m.should == "SendWithArguments<body = [FixnumLiteral<value = 1>, FixnumLiteral<value = 2>]>"
+ end
+
+ it "should proper create XSS check" do
+ dsl = Machete::DSL::Builder.build do
+ send_with_arguments do
+ name(':send_file | :send_data')
+ arguments do
+ actual_arguments do
+ array(:array) do
+ hash_literal do
+ array(:array) do
@dmajda

dmajda May 30, 2012

I wonder whether we can do without :array somehow. I don't like this much. I can't think of anything better now, so keep it, but I'll keep thinking about it more.

+ symbol_literal(value: ":disposition")
+ string_literal(string: '"inline"')
+ end
+ end
+ end
+ end
+ end
+ end
+ end
+
+ dsl.to_m.should == <<-XSS_CHECK.strip
+ SendWithArguments<name = :send_file | :send_data, arguments = ActualArguments<array = [HashLiteral<array = [SymbolLiteral<value = :disposition>, StringLiteral<string = "inline">]>]>>
+ XSS_CHECK
+
+ end
+ end
@dmajda

dmajda May 30, 2012

This doesn't really test to_m. All to_m tests should be done without any knowledge about Machete::DSL::Builder (because this is a layer above it).

What I'm missing are tests for Array#to_m (and at least trivial tests for Fixnum#to_m and NilClass#to_m).

+
+
+end

0 comments on commit 543e485

Please sign in to comment.