Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Adding trim option to ERB, closes #840 #846

Merged
merged 4 commits into from Mar 31, 2013
Merged

Adding trim option to ERB, closes #840 #846

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3874756
Adding trim option to ERB, closes #840
hron84 Mar 26, 2013
5cbe0fc
Adding test to make sure we can parse ERB format if trimming dash is …
hron84 Mar 27, 2013
267af49
Replacing condition to true
hron84 Mar 29, 2013
0bcd11b
Removing SyntaxError expectation
hron84 Mar 30, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/rspec/core/configuration_options.rb
View file
Open in desktop
Expand Up @@ -116,7 +116,7 @@ def args_from_options_file(path)
end

def options_file_as_erb_string(path)
ERB.new(File.read(path)).result(binding)
ERB.new(File.read(path), nil, '-').result(binding)
end

def custom_options_file
Expand Down
12 changes: 12 additions & 0 deletions spec/rspec/core/configuration_options_spec.rb
View file
Open in desktop
Expand Up @@ -378,6 +378,18 @@
expect(parse_options[:formatters]).to eq([['local']])
end

it "parses options file correctly if erb code has trimming options" do
File.open("./.rspec", "w") do |f|
f << "<% if 1 == 1 -%>\n"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if 1 == 1 seems like a convulated way to have an if true conditional...any reason not to just use if true? That more clearly states the intention, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. I dunno why not used true. Fixed.

f << "--format local\n"
f << "<%- end %>\n"
end

expect do
expect(parse_options[:formatters]).to eq([['local']])
end.to_not raise_error(SyntaxError)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally, it's best not to pass an error class to expect { }.not_to raise_error, because if an error is raised, but it is not a SyntaxError, this expectation will pass. Really, it shouldn't raise any kind of error, right? Given that, and the fact that you already have an expectation in there, I think I'd remove the raise_error expectation entirely.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I narrowed down to SyntaxError because if trim option is not present, SyntaxError is raised. It is an explicite sign if actual code cannot parse the .rspec file. No other way to detect missing trim options, sadly.

However, this test code is tests trim option existence, not the quality of the actual file parsing code. If another error is raised, it should be covered by another test, not by this.

But if you still want, I can remove the concrete error class.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I narrowed down to SyntaxError because if trim option is not present, SyntaxError is raised. It is an explicite sign if actual code cannot parse the .rspec file. No other way to detect missing trim options, sadly.

SyntaxError is what is raised by the ruby versions and implementations you have tried this on, but are you sure it's the error raised on all implementations and all versions (including future versions...)? If any of them raise a different kind of error, this expectation will pass even if the code doesn't work. Actually, it's worse than that. Consider what happens when you change this to:

      expect do
        expect(parse_options[:formatters]).to eq([['global']])
      end.to_not raise_error(SyntaxError)

The inner expectation fails and raises a RSpec::Expectations::ExpectationNotMetError, but because this is not a SyntaxError, it passes the outer expectation...because hey, SyntaxError was not raised. Or consider what happens if we rename the parse_options helper method but forget to update it here: the inner expression will raise a NoMethodError but that's not a SyntaxError, either, so it will still pass even though the parser isn't even being run as part of this example.

I think my preference is to remove the outer expect { }.to_not raise_error expectation entirely.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, but... my problem is future maintenance. As ERB is simply eval's the inner code, no special exception to detect template problems. The thrown SyntaxError's description is too non-informative, especially because it depends on the actual template code. Not so easy to debug what happened if SyntaxError raised.

What about if i split this into two example? Still unacceptable?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hron84 the real thing you're expecting though is no error whatsoever, not, specifically, no syntax error. Therefore if we have no to_not raise_error expectation, the spec will fail if any error is given. This helps us catch a lot more cases and actually makes the spec more resilient to future breakage (not less!). I'm personally in favour of completely removing the block like @myronmarston said.

end

context "with custom options file" do
it "ignores project and global options files" do
File.open("./.rspec", "w") {|f| f << "--format project"}
Expand Down