Clean up many test files to pass Rubocop rules #4902

Merged
merged 22 commits into from May 17, 2016

Conversation

Projects
None yet
3 participants
@brint
Contributor

brint commented May 15, 2016

This PR cleans up rubocop violations for the following files:

test/helper.rb
test/simplecov_custom_profile.rb
test/test_ansi.rb
test/test_cleaner.rb
test/test_coffeescript.rb
test/test_collections.rb
test/test_command.rb
test/test_commands_serve.rb
test/test_convertible.rb
test/test_doctor_command.rb
test/test_excerpt.rb
test/test_front_matter_defaults.rb
test/test_generated_site.rb
test/test_layout_reader.rb
test/test_liquid_extensions.rb
test/test_log_adapter.rb
test/test_new_command.rb
test/test_path_sanitization.rb
test/test_plugin_manager.rb
test/test_rdiscount.rb
test/test_redcarpet.rb

test/test_cleaner.rb
end
end
- context "not-nested directory in keep_files and similary named directory not in keep_files" do
+ context "not-nested directory in keep_files and similary named directory not "\
+ "in keep_files" do

This comment has been minimized.

@parkr

parkr May 16, 2016

Member

let's try to just shorten this name. super confusing haha. how about

non-nested directory & similarly-named directory *not* in keep_files

does that work here?

@parkr

parkr May 16, 2016

Member

let's try to just shorten this name. super confusing haha. how about

non-nested directory & similarly-named directory *not* in keep_files

does that work here?

This comment has been minimized.

@brint

brint May 17, 2016

Contributor

Fix committed locally (documenting this so I can keep track of what's done)

@brint

brint May 17, 2016

Contributor

Fix committed locally (documenting this so I can keep track of what's done)

test/test_coffeescript.rb
@@ -37,7 +37,8 @@ class TestCoffeeScript < JekyllUnitTest
end
should "write a JS file in place" do
- assert_exist @test_coffeescript_file, "Can't find the converted CoffeeScript file in the dest_dir."
+ assert_exist @test_coffeescript_file, "Can't find the converted CoffeeScript file "\
+ "in the dest_dir."

This comment has been minimized.

@parkr

parkr May 16, 2016

Member

Let's simply remove the message here – I believe our assert_exist adds an acceptable message. 😄

assert_exist @test_coffeescript_file
@parkr

parkr May 16, 2016

Member

Let's simply remove the message here – I believe our assert_exist adds an acceptable message. 😄

assert_exist @test_coffeescript_file

This comment has been minimized.

@brint

brint May 17, 2016

Contributor

Fix committed locally (documenting this so I can keep track of what's done)

@brint

brint May 17, 2016

Contributor

Fix committed locally (documenting this so I can keep track of what's done)

test/test_collections.rb
@@ -113,7 +113,8 @@ class TestCollections < JekyllUnitTest
@collection = @site.collections["methods"]
end
- should "create a Hash on Site with the label mapped to the instance of the Collection" do
+ should "create a Hash on Site with the label mapped to the instance of the "\
+ "Collection" do

This comment has been minimized.

@parkr

parkr May 16, 2016

Member

This is strangely specific – how about:

should "create a Hash mapping label to Collection instance" do
@parkr

parkr May 16, 2016

Member

This is strangely specific – how about:

should "create a Hash mapping label to Collection instance" do

This comment has been minimized.

@brint

brint May 17, 2016

Contributor

Fix committed locally (documenting this so I can keep track of what's done)

@brint

brint May 17, 2016

Contributor

Fix committed locally (documenting this so I can keep track of what's done)

test/test_collections.rb
end
end
- should "not include files which start with an underscore in the base collection directory" do
+ should "not include files which start with an underscore in the base collection "\
+ "directory" do

This comment has been minimized.

@parkr

parkr May 16, 2016

Member

Putting just one or two words on the next line are not great... How about:

should "not include files from base dir which start with an underscore" do

Does that work?

@parkr

parkr May 16, 2016

Member

Putting just one or two words on the next line are not great... How about:

should "not include files from base dir which start with an underscore" do

Does that work?

This comment has been minimized.

@brint

brint May 17, 2016

Contributor

Fix committed locally (documenting this so I can keep track of what's done)

@brint

brint May 17, 2016

Contributor

Fix committed locally (documenting this so I can keep track of what's done)

test/test_collections.rb
@@ -184,7 +187,8 @@ class TestCollections < JekyllUnitTest
refute_includes @collection.filtered_entries, "/um_hi.md"
end
- should "include the symlinked file in the list of docs as it resolves to inside site.source" do
+ should "include the symlinked file in the list of docs as it resolves to inside "\
+ "site.source" do

This comment has been minimized.

@parkr

parkr May 16, 2016

Member
should "include the symlinked file from site.source in the list of docs" do

Does that fit?

@parkr

parkr May 16, 2016

Member
should "include the symlinked file from site.source in the list of docs" do

Does that fit?

This comment has been minimized.

@brint

brint May 17, 2016

Contributor

Fix committed locally (documenting this so I can keep track of what's done)

@brint

brint May 17, 2016

Contributor

Fix committed locally (documenting this so I can keep track of what's done)

test/test_excerpt.rb
- collection: @site.posts
+ Document.new(@site.in_source_dir(File.join("_posts", file)), {
+ :site => @site,
+ :collection => @site.posts

This comment has been minimized.

@parkr

parkr May 16, 2016

Member

Please align the @site's here. I applied that in 2caff75. Please rebase on master. 😄

@parkr

parkr May 16, 2016

Member

Please align the @site's here. I applied that in 2caff75. Please rebase on master. 😄

This comment has been minimized.

@brint

brint May 17, 2016

Contributor

Fix committed locally (documenting this so I can keep track of what's done)

There were ~28 of these offenses after rebasing :)

@brint

brint May 17, 2016

Contributor

Fix committed locally (documenting this so I can keep track of what's done)

There were ~28 of these offenses after rebasing :)

test/test_excerpt.rb
}).tap(&:read)
end
def do_render(document)
- @site.layouts = { "default" => Layout.new(@site, source_dir('_layouts'), "simple.html")}
+ @site.layouts = { "default" => Layout.new(@site, source_dir("_layouts"),
+ "simple.html") }

This comment has been minimized.

@parkr

parkr May 16, 2016

Member

I'd move the key down instead of the one argument:

@site.layouts = {
  "default" => Layout.new(@site, source_dir("_layouts"), "simple.html")
}
@parkr

parkr May 16, 2016

Member

I'd move the key down instead of the one argument:

@site.layouts = {
  "default" => Layout.new(@site, source_dir("_layouts"), "simple.html")
}

This comment has been minimized.

@brint

brint May 17, 2016

Contributor

Fix committed locally (documenting this so I can keep track of what's done)

Yes, this is way better, I think I was in cruise control at this point.

@brint

brint May 17, 2016

Contributor

Fix committed locally (documenting this so I can keep track of what's done)

Yes, this is way better, I think I was in cruise control at this point.

test/test_excerpt.rb
@@ -78,19 +78,21 @@ def do_render(document)
context "#to_liquid" do
should "contain the proper page data to mimick the post liquid" do
assert_equal "Post Excerpt with Layout", @excerpt.to_liquid["title"]
- assert_equal "/bar/baz/z_category/mixedcase/2013/07/22/post-excerpt-with-layout.html", @excerpt.to_liquid["url"]
+ assert_equal "/bar/baz/z_category/mixedcase/2013/07/22/"\
+ "post-excerpt-with-layout.html", @excerpt.to_liquid["url"]

This comment has been minimized.

@parkr

parkr May 16, 2016

Member

Is it too long to keep the rest of that string in the line above?

@parkr

parkr May 16, 2016

Member

Is it too long to keep the rest of that string in the line above?

This comment has been minimized.

@brint

brint May 17, 2016

Contributor

It hits the line length limit at the m in .html:

test/test_excerpt.rb:82:91: W: Metrics/LineLength: Line is too long. [120/90]
        assert_equal "/bar/baz/z_category/mixedcase/2013/07/22/post-excerpt-with-layout.html", @excerpt.to_liquid["url"]
                                                                                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@brint

brint May 17, 2016

Contributor

It hits the line length limit at the m in .html:

test/test_excerpt.rb:82:91: W: Metrics/LineLength: Line is too long. [120/90]
        assert_equal "/bar/baz/z_category/mixedcase/2013/07/22/post-excerpt-with-layout.html", @excerpt.to_liquid["url"]
                                                                                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

This comment has been minimized.

@parkr

parkr May 17, 2016

Member

@brint Hm... what about:

expected_url = "/bar/baz/z_category/mixedcase/2013/07/22/post-excerpt-with-layout.html"
assert_equal expected_url, @excerpt.to_liquid["url"]

?

@parkr

parkr May 17, 2016

Member

@brint Hm... what about:

expected_url = "/bar/baz/z_category/mixedcase/2013/07/22/post-excerpt-with-layout.html"
assert_equal expected_url, @excerpt.to_liquid["url"]

?

This comment has been minimized.

@brint

brint May 17, 2016

Contributor

@parkr how do you feel about this?

url = "/bar/baz/z_category/mixedcase/2013/07/22/post-excerpt-with-layout.html"
assert_equal url, @excerpt.to_liquid["url"]

Using expected or expected_url triggers the line limit by 1 and 5 chars respectively. Upside of this setup, one line for the path to the file. Downside, inconsistent with the naming of variables in other tests.

@brint

brint May 17, 2016

Contributor

@parkr how do you feel about this?

url = "/bar/baz/z_category/mixedcase/2013/07/22/post-excerpt-with-layout.html"
assert_equal url, @excerpt.to_liquid["url"]

Using expected or expected_url triggers the line limit by 1 and 5 chars respectively. Upside of this setup, one line for the path to the file. Downside, inconsistent with the naming of variables in other tests.

test/test_excerpt.rb
context "before render" do
should "be the first paragraph of the page" do
- assert_equal "First paragraph with [link ref][link].\n\n[link]: http://www.jekyllrb.com/", @excerpt.content
+ assert_equal "First paragraph with [link ref][link].\n\n[link]:"\
+ " http://www.jekyllrb.com/", @excerpt.content

This comment has been minimized.

@parkr

parkr May 16, 2016

Member

Let's move this string to some other assignment like

expected = "First paragraph..."
assert_equal expected, @excerpt.content

Splitting up the string like that isn't very readable.

@parkr

parkr May 16, 2016

Member

Let's move this string to some other assignment like

expected = "First paragraph..."
assert_equal expected, @excerpt.content

Splitting up the string like that isn't very readable.

This comment has been minimized.

@brint

brint May 17, 2016

Contributor

Committing the following, as one line it comes back 7 chars over the limit:

          expected = "First paragraph with [link ref][link].\n\n[link]: "\
                     "http://www.jekyllrb.com/"
          assert_equal expected, @excerpt.content
@brint

brint May 17, 2016

Contributor

Committing the following, as one line it comes back 7 chars over the limit:

          expected = "First paragraph with [link ref][link].\n\n[link]: "\
                     "http://www.jekyllrb.com/"
          assert_equal expected, @excerpt.content
test/test_excerpt.rb
end
should "be the first paragraph of the page" do
- assert_equal "<p>First paragraph with <a href=\"http://www.jekyllrb.com/\">link ref</a>.</p>\n\n", @extracted_excerpt.output
+ assert_equal "<p>First paragraph with <a href=\"http://www.jekyllrb.com/\">"\
+ "link ref</a>.</p>\n\n", @extracted_excerpt.output

This comment has been minimized.

@parkr

parkr May 16, 2016

Member

Same as above – please put the string back onto one line and use some temporary variable like expected to hold it.

@parkr

parkr May 16, 2016

Member

Same as above – please put the string back onto one line and use some temporary variable like expected to hold it.

This comment has been minimized.

@brint

brint May 17, 2016

Contributor

Same as above:

          expected = "<p>First paragraph with <a href=\"http://www.jekyllrb.com/\">link "\
                     "ref</a>.</p>\n\n"
          assert_equal expected, @extracted_excerpt.output
@brint

brint May 17, 2016

Contributor

Same as above:

          expected = "<p>First paragraph with <a href=\"http://www.jekyllrb.com/\">link "\
                     "ref</a>.</p>\n\n"
          assert_equal expected, @extracted_excerpt.output
test/test_log_adapter.rb
- allow(writer).to receive(:debug).with('topic '.rjust(20) + 'log message').and_return(true)
- assert logger.debug('topic', 'log message')
+ allow(writer).to receive(:debug).with("topic ".rjust(20) +
+ "log message").and_return(true)

This comment has been minimized.

@parkr

parkr May 16, 2016

Member

Let's move the whole .with(..).and_return(true) down to line 65.

@parkr

parkr May 16, 2016

Member

Let's move the whole .with(..).and_return(true) down to line 65.

This comment has been minimized.

@brint

brint May 17, 2016

Contributor

Fix committed locally (documenting this so I can keep track of what's done)

@brint

brint May 17, 2016

Contributor

Fix committed locally (documenting this so I can keep track of what's done)

test/test_log_adapter.rb
- allow(writer).to receive(:info).with('topic '.rjust(20) + 'log message').and_return(true)
- assert logger.info('topic', 'log message')
+ allow(writer).to receive(:info).with("topic ".rjust(20) +
+ "log message").and_return(true)

This comment has been minimized.

@parkr

parkr May 16, 2016

Member

Same here – move the .with... down.

@parkr

parkr May 16, 2016

Member

Same here – move the .with... down.

This comment has been minimized.

@brint

brint May 17, 2016

Contributor

Fix committed locally (documenting this so I can keep track of what's done)

@brint

brint May 17, 2016

Contributor

Fix committed locally (documenting this so I can keep track of what's done)

test/test_log_adapter.rb
- allow(writer).to receive(:warn).with('topic '.rjust(20) + 'log message').and_return(true)
- assert logger.warn('topic', 'log message')
+ allow(writer).to receive(:warn).with("topic ".rjust(20) +
+ "log message").and_return(true)

This comment has been minimized.

@parkr

parkr May 16, 2016

Member

And here – move the .with... down.

@parkr

parkr May 16, 2016

Member

And here – move the .with... down.

This comment has been minimized.

@brint

brint May 17, 2016

Contributor

Fix committed locally (documenting this so I can keep track of what's done)

@brint

brint May 17, 2016

Contributor

Fix committed locally (documenting this so I can keep track of what's done)

test/test_log_adapter.rb
- allow(writer).to receive(:error).with('topic '.rjust(20) + 'log message').and_return(true)
- assert logger.error('topic', 'log message')
+ allow(writer).to receive(:error).with("topic ".rjust(20) +
+ "log message").and_return(true)

This comment has been minimized.

@parkr

parkr May 16, 2016

Member

And here – move the .with... down

@parkr

parkr May 16, 2016

Member

And here – move the .with... down

This comment has been minimized.

@brint

brint May 17, 2016

Contributor

Fix committed locally (documenting this so I can keep track of what's done)

@brint

brint May 17, 2016

Contributor

Fix committed locally (documenting this so I can keep track of what's done)

test/test_rdiscount.rb
+ "markdown" => "rdiscount",
+ "rdiscount" => {
+ "toc_token" => "{:toc}",
+ "extensions" => %w(smart generate_toc)

This comment has been minimized.

@parkr

parkr May 16, 2016

Member

This will change with the new EnforceHashRocketStyle: table setting.

@parkr

parkr May 16, 2016

Member

This will change with the new EnforceHashRocketStyle: table setting.

This comment has been minimized.

@brint

brint May 17, 2016

Contributor

Fix committed locally (documenting this so I can keep track of what's done)

@brint

brint May 17, 2016

Contributor

Fix committed locally (documenting this so I can keep track of what's done)

@parkr parkr referenced this pull request May 16, 2016

Closed

Make jekyll/jekyll code compliant with rubocop rules #4885

114 of 115 tasks complete
@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr May 16, 2016

Member

Hey @brint, thanks so much for your pull request! I have some things I'd like for you to fix up before we merge this, but it's looking great! We'll merge it once the comments I left are addressed. Thank you! 😄

Member

parkr commented May 16, 2016

Hey @brint, thanks so much for your pull request! I have some things I'd like for you to fix up before we merge this, but it's looking great! We'll merge it once the comments I left are addressed. Thank you! 😄

@brint

This comment has been minimized.

Show comment
Hide comment
@brint

brint May 17, 2016

Contributor

Sounds good! Doing the cleanup, I've rebased to pick up some of the new rules. Some of these lines were one or two characters over the 90

Contributor

brint commented May 17, 2016

Sounds good! Doing the cleanup, I've rebased to pick up some of the new rules. Some of these lines were one or two characters over the 90

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr May 17, 2016

Member

Doing the cleanup, I've rebased to pick up some of the new rules. Some of these lines were one or two characters over the 90

Ah ok. Feel free to rephrase some of the suggestions so they're less than 90 characters. Thank you!

Member

parkr commented May 17, 2016

Doing the cleanup, I've rebased to pick up some of the new rules. Some of these lines were one or two characters over the 90

Ah ok. Feel free to rephrase some of the suggestions so they're less than 90 characters. Thank you!

@parkr parkr changed the title from Rubocop test cleanups for #4885 to Clean up many test files to pass Rubocop rules May 17, 2016

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr May 17, 2016

Member

Lookin' good to me, thanks @brint!

@jekyllbot: merge +dev

Member

parkr commented May 17, 2016

Lookin' good to me, thanks @brint!

@jekyllbot: merge +dev

@jekyllbot jekyllbot merged commit 9e92061 into jekyll:master May 17, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

jekyllbot added a commit that referenced this pull request May 17, 2016

@brint brint deleted the brint:rubocop_cleanup_tests branch May 17, 2016

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