CSS Tests with RSpec #54

Closed
wants to merge 12 commits into
from

Projects

None yet

5 participants

@kaishin
kaishin commented Jan 24, 2013

No description provided.

Reda Lemeden added some commits Dec 29, 2012
@mike-burns mike-burns commented on an outdated diff Jan 24, 2013
spec/support/matchers/have_rule.rb
@@ -0,0 +1,25 @@
+RSpec::Matchers.define :have_rule do |expected|
+ match do |actual|
+ @rules = get_rules(actual)? get_rules(actual) : []
@mike-burns
mike-burns Jan 24, 2013 Member
@rules = get_rules(actual) || []
@mike-burns mike-burns commented on an outdated diff Jan 24, 2013
spec/support/matchers/have_rule.rb
@@ -0,0 +1,25 @@
+RSpec::Matchers.define :have_rule do |expected|
+ match do |actual|
+ @rules = get_rules(actual)? get_rules(actual) : []
+ @rules.include? expected
+ end
+
+ failure_message_for_should do |actual|
+ if @rules.empty?
+ "selector #{actual} not found"
+ else
+ "expected #{actual} to have rule \"#{expected}\""
@mike-burns
mike-burns Jan 24, 2013 Member
%{expected #{actual} to have rule "#{expected}"}
@mike-burns mike-burns commented on an outdated diff Jan 24, 2013
spec/support/bourbon_support.rb
@@ -0,0 +1,9 @@
+module BourbonSupport
+ def install_bourbon_files
+ system("bourbon install --path test > /dev/null")
@mike-burns
mike-burns Jan 24, 2013 Member

Indentation.

@mike-burns mike-burns and 2 others commented on an outdated diff Jan 24, 2013
@@ -26,4 +26,9 @@ Neat is an open source grid framework built on top of Bourbon with the aim of be
s.add_development_dependency('aruba', '~> 0.4')
s.add_development_dependency('rake')
+ s.add_development_dependency('css_parser')
+ s.add_development_dependency('rspec')
+ s.add_development_dependency('rdoc')
+ s.add_development_dependency('jeweler')
@mike-burns
mike-burns Jan 24, 2013 Member

What are you using Jewler for that the Bundler rake tasks don't cover?

@calebthompson
calebthompson Jan 24, 2013

Why are you using Jewler?

@kaishin
kaishin Jan 24, 2013

Good catch. I played with Jeweler for a little while and forgot to remove it.

@mike-burns mike-burns and 3 others commented on an outdated diff Jan 24, 2013
spec/support/parser_support.rb
@@ -0,0 +1,10 @@
+module ParserSupport
+ def initialize_parser
+ $parser = CssParser::Parser.new
@mike-burns
mike-burns Jan 24, 2013 Member

Why is this a global variable ($) instead of an instance variable (@)?

@kaishin
kaishin Jan 24, 2013

I had little success in accessing @parser from inside the custom matcher. If you have a fix/alternate solution I'd love to hear it.

@calebthompson
calebthompson Jan 24, 2013

You can make @parser available outside with attr_accessor :parser

@kaishin
kaishin Jan 24, 2013

@calebthompson Aha! So I added attr_accessor :parser to module ParserSupport (above the initialization method) but the matcher still throws an undefined method error on https://github.com/thoughtbot/neat/pull/54/files#L8R16

@calebthompson
calebthompson Jan 24, 2013

Oh, module. Modules don't have state, and so can't have attributes without ugly hacks we won't go into.

Given we are in a method with "initialize" in the name, I would suggest a refactor that looks like this, where ParserSupport functionality is rolled into CssParser::Parser.

https://github.com/thoughtbot/shoulda-matchers/blob/master/lib/shoulda/matchers/active_record/association_matcher.rb#L67-L71

@calebthompson
calebthompson Jan 24, 2013

The example I linked would be in spec/support/masters/has_rule.rb

@kaishin
kaishin Jan 24, 2013

Are you suggesting that I create a new class inside the matcher? I'm not sure how that would work given that I also need to call:

  before(:all) do
    parse_css_file(identifier)
  end

...inside the spec, since I only need the file to be parsed once.
Basically all I need is a common CSSParser::Parser instance that can accessed from both the spec and the matcher.

@jferris
jferris Jan 24, 2013 Member

You can use the Singleton pattern if you need to only have one reference to CSSParser.

Wikipedia has some high-level stuff on it: http://en.wikipedia.org/wiki/Singleton_pattern

There's also a Ruby module that helps implement it: http://www.ruby-doc.org/stdlib-1.9.3/libdoc/singleton/rdoc/Singleton.html

@kaishin
kaishin Jan 24, 2013

I never used singletons in Ruby (used them in Obj-C) but I had thought of it at some point. I'll try to come up with something.

@kaishin
kaishin Jan 25, 2013

@jferris Since CssParser is a 3rd-party module, would something like this be legal?

class CustomParser < CssParser::Parser
  include Singleton

  def initialize
    super
  end
end
@jferris
jferris Jan 25, 2013 Member

Sorry, I didn't notice that you were initializing a class you didn't write.

You could definitely use a subclass. As long as the CssParser code doesn't instantiate another CssParser directly for any reason, it will work fine.

An easier solution might be to write a class method on the module:

module ParserSupport
  def self.parser
    @parser ||= CssParser::Parser.new
  end

  def parser
    ParserSupport.parser
  end
end
@calebthompson calebthompson and 1 other commented on an outdated diff Jan 24, 2013
spec/support/sass_support.rb
@@ -0,0 +1,10 @@
+module SassSupport
+ def generate_css
+ _mkdir('tmp')
+ system('sass --update test:tmp --style expanded > /dev/null')
@calebthompson
calebthompson Jan 24, 2013

If you just use backticks, ruby will not print the output of the command.

`sass --update test:tmp --style expanded`
@kaishin
kaishin Jan 24, 2013

Thanks Caleb, I didn't know about this.

@gabebw gabebw and 1 other commented on an outdated diff Jan 24, 2013
spec/support/matchers/have_rule.rb
+ @rules.include? expected
+ end
+
+ failure_message_for_should do |actual|
+ if @rules.empty?
+ "selector #{actual} not found"
+ else
+ "expected #{actual} to have rule \"#{expected}\""
+ end
+ end
+
+ def get_rules(actual)
+ style_block = $parser.find_by_selector(actual)
+ unless style_block.empty?
+ rules = style_block[0].split(';')
+ rules.each do |rule|
@gabebw
gabebw Jan 24, 2013 Member

You can replace this each block, plus the rules on line 22, with just this:

rules.map do |rule|
  # Note lack of exclamation point after gsub
  rule.gsub(/^(\s)|(\s)$/, "")
end
@mike-burns
mike-burns Jan 24, 2013 Member

Good catch! 🚎

@gabebw gabebw commented on an outdated diff Jan 24, 2013
spec/neat/omega_spec.rb
@@ -0,0 +1,41 @@
+require File.expand_path('spec/spec_helper')
@gabebw
gabebw Jan 24, 2013 Member

This can/should just be require 'spec_helper'.

@gabebw gabebw commented on an outdated diff Jan 24, 2013
Bundler::GemHelper.install_tasks
+
+RSpec::Core::RakeTask.new(:spec) do |spec|
@gabebw
gabebw Jan 24, 2013 Member

You don't need the do/end block, RSpec will automatically find all files in the spec/ dir that end in _spec.rb:

RSpec::Core::RakeTask.new(:spec)
@calebthompson calebthompson commented on an outdated diff Jan 24, 2013
spec/support/parser_support.rb
@@ -0,0 +1,10 @@
+module ParserSupport
+ def initialize_parser
+ $parser = CssParser::Parser.new
+ end
+
+ def parse_css_file(identifier)
+ initialize_parser if !$parser
@calebthompson
calebthompson Jan 24, 2013

We try to avoid trailing conditionals, as well as if !

Try

if @parser.nil?
  initialize_parser
end

or

@parser || initialize_parser
@kaishin kaishin referenced this pull request Jan 24, 2013
Closed

Add tests #50

@kaishin
kaishin commented Jan 25, 2013

This is how I ended up implementing it:

module ParserSupport
  def self.parser
    @parser ||= CssParser::Parser.new
  end

  def self.parse_file(identifier)
    self.parser.load_file!("tmp/#{identifier}.css")
  end
end
@kaishin
kaishin commented Jan 25, 2013

Can I get this merged if it's good to go?

@calebthompson calebthompson commented on an outdated diff Jan 26, 2013
spec/support/matchers/have_rule.rb
+
+ failure_message_for_should do |actual|
+ if @rules.empty?
+ %{selector #{actual} not found}
+ else
+ %{expected #{actual} to have rule "#{expected}"}
+ end
+ end
+
+ def get_rules(actual)
+ style_block = ParserSupport.parser.find_by_selector(actual)
+ unless style_block.empty?
+ rules = style_block[0].split(';')
+ rules.map do |rule|
+ rule.gsub(/^(\s)|(\s)$/, "")
+ end
@calebthompson
calebthompson Jan 26, 2013

It looks like lines 19-21 are responsible for killing whitespace in the rules. If that's correct, String has a method, strip, which will do this for you. This block (which is a little convoluted) can be changed to rules.map(&:strip).

I will say that strip will remove tabs, newlines, and other whitespace as well, some of which \s isn't doing now. I'm pretty sure you are iterating by line at this point, so that should be okay.

Reda Lemeden added some commits Jan 28, 2013
@kaishin kaishin pushed a commit that closed this pull request Jan 28, 2013
Reda Lemeden Test Neat features using RSpec
Closes #54
89ca2d4
@kaishin kaishin closed this in 89ca2d4 Jan 28, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment