First BDD/RSpec tests, would like some review regarding idioms, conventions and style

Question

This is my first attempt at BDD and RSpec. I'm more of a C# xUnit kind of guy.

I wrote this class while developing a 2D random tile map generator (just for fun). The project is still ongoing.

I would particularly like some review on:

• Test granularity/scope in relation to BDD
• Use of RSpec
• Are these too much like unit tests?
• Adherence to Ruby and RSpec idioms, conventions, coding style

I am also open to any other kind of comments and review.

require './map_factory'

describe MapFactory, "map creation" do
    before(:each) do
        @map_factory = MapFactory.new
    end

  it "should return a new Map instance" do
    map = @map_factory.make(20, 20)  
    map.should be_an_instance_of(Map)
  end

  it "should be a map of the specified size" do
    map = @map_factory.make(20, 20)
    map.width.should equal(20)
    map.height.should equal(20)
    map.tiles.length.should equal(400)
  end

  it "should be an island" do
    map = @map_factory.make(20, 20)

    # an island should have water all around it
    for x in 0...map.width
        map.tile_at(x, 0).type.should eq(:water)
        map.tile_at(x, 19).type.should eq(:water)
    end

    for y in 0...map.height
        map.tile_at(0, y).type.should eq(:water)
        map.tile_at(19, y).type.should eq(:water)
    end
  end

  it "should have more than half the tiles be landmass" do  
    map = @map_factory.make(40, 40)

    number_of_water_tiles = 0
    number_of_land_tiles = 0

    map.tiles.each do |tile|
        if tile.type == :water then
            number_of_water_tiles += 1
        else
            number_of_land_tiles += 1
        end
    end

    number_of_land_tiles.should be > number_of_water_tiles
  end
end

Answer 1

I am no RSpec pro yet, but here are a few things I would have done differently (see the comments):

require './map_factory'

describe MapFactory, "map creation" do
  before(:each) do
      @map_factory = MapFactory.new
  end 

  # I would put those 3 tests in the same context to DRY up things
  context "20 x 20 map" do
    before(:each) do
      map_factory = MapFactory.new # EDIT: this was an instance variable, which is useless here
      @map = map_factory.make(20, 20) # all three following tests used that, so why not use an instance variable here?
    end

    it "should return a new Map instance" do
      @map.should be_an_instance_of(Map)
    end

    it "should be a map of the specified size" do
      @map.width.should equal(20)
      @map.height.should equal(20)
      @map.tiles.length.should equal(400)
    end

    # you had a comment here, maybe it could be in the test description?
    it "should be an island and have water all around it" do
      # Enforce the use of Ruby ranges and iterator
      ([email protected]).each do |x| 
        @map.tile_at(x, 0).type.should eq(:water)
        @map.tile_at(x, 19).type.should eq(:water)
      end

      ([email protected]).each do |y|
        @map.tile_at(0, y).type.should eq(:water)
        @map.tile_at(19, y).type.should eq(:water)
      end
    end
  end

  it "should have more than half the tiles be landmass" do  
    number_of_water_tiles = 0
    number_of_land_tiles = 0

    @map.tiles.each do |tile|
        if tile.type == :water # remove "then" here as it is not needed
            number_of_water_tiles += 1
        else
            number_of_land_tiles += 1
        end
    end

    number_of_land_tiles.should be > number_of_water_tiles
  end
end

edit: see in code comments

Answer 2

Your tests are good. But they can be better)

• At first, use one should for one example( it ). Your test falls when first matcher in order return false. And conditions below won't be checked. It's good for finding bugs and refactoring. Green test is not the reason of thinking that your code haven't got any bugs :) Read this

• Don’t begin tests(examples) names with the word ‘should’ for readability. Results should be easy to read.

• Some developers very like custom matchers for readable and clean code. It's very cool, but it is not essential often

Answer 3

A small hint: the count method could clean up one of your test case, more or less as such:

it "should have more than half the tiles be landmass" do  
    map = @map_factory.make(40, 40)
    number_of_water_tiles = map.tiles.count{|tile| tile.type == :water}
    number_of_land_tiles = map.tiles.size - number_of_water_tiles
    number_of_land_tiles.should be > (map.tiles.size / 2)
  end
end

Also your map could go in the before section too imho.