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

Improve features / tests code #1493

Merged
merged 4 commits into from Oct 24, 2013

Conversation

Projects
None yet
5 participants
@maul-esel
Contributor

maul-esel commented Sep 2, 2013

Improves the cucumber step definitions by reducing code duplication and using assert_match for better error messages.

@@ -142,11 +142,11 @@
end
Then /^I should see "(.*)" in "(.*)"$/ do |text, file|
assert Regexp.new(text).match(File.open(file).readlines.join)
assert Regexp.new(text).match(File.read(file))

This comment has been minimized.

@parkr

parkr Sep 4, 2013

Member

This makes testing on systems that use \r\n inconsistent :/

This comment has been minimized.

@mattr-

mattr- Sep 4, 2013

Member

hmm. wasn't aware of that.

On Wed, Sep 4, 2013 at 9:45 AM, Parker Moore notifications@github.comwrote:

In features/step_definitions/jekyll_steps.rb:

@@ -142,11 +142,11 @@
end

Then /^I should see "(.)" in "(.)"$/ do |text, file|

  • assert Regexp.new(text).match(File.open(file).readlines.join)
  • assert Regexp.new(text).match(File.read(file))

This makes testing on systems that use \r\n inconsistent :/


Reply to this email directly or view it on GitHubhttps://github.com//pull/1493/files#r6155478
.

This comment has been minimized.

@parkr

parkr Sep 4, 2013

Member

The structure is different, anyhow. If we don't care then I guess we don't care.

This comment has been minimized.

@maul-esel

maul-esel Sep 5, 2013

Contributor

I didn't know that. Maybe I could just introduce a new function in features/support/env.rb that encapsulates this construct?

This comment has been minimized.

@mattr-

mattr- Sep 5, 2013

Member

I'm almost never going to turn down having a new function for something, so
I'm cool if you want to go ahead and do that. 😃

This comment has been minimized.

@parkr

parkr Sep 7, 2013

Member

Once we have the new method for this, I'm 👍.

Revert to #readlines#join, but enclose it in a function
This is necessary to preserve the handling
of \r\n and \n line endings.
@maul-esel

This comment has been minimized.

Contributor

maul-esel commented Sep 7, 2013

@parkr, @mattr-: now using a read_file function.

@@ -35,5 +35,11 @@ def location(folder, direction)
[before || '.', after || '.']
end
def read_file(path)

This comment has been minimized.

@parkr

parkr Sep 9, 2013

Member

maybe file_contents?

This comment has been minimized.

@maul-esel

maul-esel Sep 10, 2013

Contributor

Done 😀

@maul-esel

This comment has been minimized.

Contributor

maul-esel commented Oct 7, 2013

I'd consider this ready-to-merge. Anything holding it back?

@troyswanson

This comment has been minimized.

Member

troyswanson commented Oct 7, 2013

Nicely done!

@parkr

This comment has been minimized.

Member

parkr commented Oct 7, 2013

👍 from me! @mattr-?

mattr- added a commit that referenced this pull request Oct 24, 2013

@mattr- mattr- merged commit fb6f8c1 into jekyll:master Oct 24, 2013

1 check passed

default The Travis CI build passed
Details

mattr- added a commit that referenced this pull request Oct 24, 2013

@maul-esel maul-esel deleted the maul-esel:feature-improvements branch Oct 24, 2013

@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.