City class and spec for testing it

Question

I have written a class and spec for testing my class. Can anyone determine whether or not this is a correct and good spec?

Class:

module Sitemap
=begin
Class Name: Cities
Function: This class is responsible for getting the data to create the sitemap 
for each country according to cities. 
=end
  class City
    attr_accessor :country_version, :directory, :country_host, :locale
    def initialize(country_version, directory, country_host,locale)
      @country_version = country_version
      @directory = directory
      @country_host = country_host
      @locale = locale
    end

    def get_data
      ::City.find_each(:conditions => {:country_version_id => @country_version.id}) do |city|
        I18n.locale=(@locale)
        yield entry(city)
      end
    end

    private
    def entry(city)
      {
        :loc => ActionController::Integration::Session.new.url_for(:controller => 'cities', :action => 'show', :city_name => city.name, :host => @country_host.value),
        :changefreq => 0.8,
        :priority => 'monthly',
        :lastmod => city.updated_at
      }
    end
  end
end 

Spec:

require File.join(File.dirname(__FILE__), "/../spec_helper" )
module Sitemap
  describe City do

    before :all do
    @city = City.new :country_version, :directory, :country_host, :locale
    end

    describe "#new" do
      it "takes four parameters and returns a City object" do
        @city.should be_an_instance_of City
      end
    end

    describe "#country_version" do
      it "returns the correct country version" do
        @city.country_version.should eql :country_version
      end
    end

    describe "#directory" do
      it "returns the correct filepath" do
        @city.directory.should eql :directory
      end
    end

    describe "#country_host" do
      it "returns the correct country host" do
        @city.country_host.should eql :country_host
      end
    end

    describe "#locale" do
      it "returns the correct locale for translation" do
        @city.locale.should eql :locale
      end
    end

    describe "#get_data" do
      it "returns collections for xml entries" do
        @city.get_data.should eql ::City
      end
    end 

  end
end

Answer 1

Here are a few things I thought of:

1. Use let when you can, it is a pretty versatile function for testing.

   before :all
     @city = City.new :country_version, :directory, :country_host, :locale
   end
   

   Can be rewritten as:

   let(:city) { @city = City.new :country_version, :directory, :country_host, :locale }
   

   This will only call city when you use it.

2. I don't typically test accessors/intatiation but if I did, I would probably test it with a specify block, it is also good for most tests where the description is "it should do something correctly":

   This is nice for simple tests that explain themselves

   specify { city.should be_an_instance_of City }

3. When testing, I try to think in terms of cases: Ex:

   it "should act in manner x when no cities are found"
   it "should act in manner y when 1 city is found"
   it "should act in manner z when n cities are found"
   

   I find testing 0, 1, n catches a lot of potential errors

4. The testing gods expect you to test first, then write code. If you don't :D, then make sure that your test fails in the way you expect it to.