tests: fix code style issues #587

eirinikos · 2016-07-25T20:07:38Z

Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same change?
Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes? Here's an example.
Have you successfully run brew tests with your changes locally?

This change fixes many of the existing bugs reported by brew style for Homebrew/test.

33 bugs remain; many of them are one of the following:

Method definitions must not be nested. Use lambda instead.
- these method definitions all (or mostly) seem to be enclosed within formula do blocks; does it make sense to convert these to lambdas? (I haven't yet figured out a way to do so that passes brew tests.)
Use snake_case for method names.
- examples of these methods include test_check_DYLD_vars and test_create_DATA; it seems like these acronyms are best kept as-is.
Prefer to_s over string interpolation.
- these bugs are easy to fix, but many instances of string interpolation seem to be tolerated by brew style (e.g., in test_integration_cmds; why is that?

I'm sensing that Rubocop's opinions are meant to be guidelines (rather than gospel), but maybe I'm wrong!

🎈

DomT4 · 2016-07-25T22:40:16Z

Library/Homebrew/test/test_tap.rb

@@ -154,7 +154,7 @@ def test_git_variant

    assert_equal "e1893a6bd191ba895c71b652ff8376a6114c7fa7", @tap.git_head
    assert_equal "e189", @tap.git_short_head
-    assert_match %r{years ago}, @tap.git_last_commit
+    assert_match /years ago/, @tap.git_last_commit


Could this just be "years ago" & avoid // entirely given we're not actually using a regex here or am I missing something?

Makes sense to me!

UniqMartin · 2016-07-27T04:36:41Z

This change fixes many of the existing bugs reported by brew style for Homebrew/test.

Sorry if I'm being a bit pedantic here, but “bugs” feels like to strong of a word. There's nothing inherently wrong or broken about the code, it's just that it doesn't fully conform to certain code style guidelines.

Aside from this there's also a minor issue with the commit subject (and PR title). The way I tend to read them is <area of changed code>: <what was changed> and in this spirit a more fitting subject would be tests: fix code style issues or something similar.

Method definitions must not be nested. Use lambda instead.

these method definitions all (or mostly) seem to be enclosed within formula do blocks; does it make sense to convert these to lambdas? (I haven't yet figured out a way to do so that passes brew tests.)

This is one of those cases where refactoring is unlikely to make sense and RuboCop suggests a solution that doesn't apply because it fails to understand the full context of the surrounding code. Depending on whether this affects individual lines or short blocks of code, the best solution is to suppress those complaints by temporarily disabling one or more cops, as detailed in the RuboCop documentation.

Use snake_case for method names.

examples of these methods include test_check_DYLD_vars and test_create_DATA; it seems like these acronyms are best kept as-is.

True. Silencing the relevant cop is the best way to address this and to drop the violation count.

Prefer to_s over string interpolation.

these bugs are easy to fix, but many instances of string interpolation seem to be tolerated by brew style (e.g., in test_integration_cmds; why is that?

This mostly sounds like a misunderstanding to me, i.e. in which contexts this suggestion applies. "#{something}" is an unnecessary use of string interpolation because there's only the variable something and no surrounding text or other variables, thus something (if it is a string) or something.to_s (if it can be converted to a string) is more appropriate. "#{something}\n" instead is a perfectly fine use of string interpolation.

I'm sensing that Rubocop's opinions are meant to be guidelines (rather than gospel), but maybe I'm wrong!

Indeed, they are guidelines and where possible, we have tweaked the settings because our preferred style doesn't fully align with what RuboCop considers to be the best default. It can sometimes make sense to tweak a rule instead of “fixing” our code, though most suggestions and defaults are good (and just show how old and diverse some of our code base is).

UniqMartin · 2016-07-27T04:48:20Z

Library/Homebrew/test/test_diagnostic.rb

@@ -142,7 +142,7 @@ def test_check_user_path_bin
  def test_check_user_path_sbin
    sbin = HOMEBREW_PREFIX/"sbin"
    ENV["PATH"] = "#{HOMEBREW_PREFIX}/bin#{File::PATH_SEPARATOR}" +
-      ENV["PATH"].gsub(%r{(?:^|#{File::PATH_SEPARATOR})#{sbin}}, "")
+                  ENV["PATH"].gsub(/(?:^|File::PATH_SEPARATOR.to_s)sbin.to_s/, "")


The substitution of #{File::PATH_SEPARATOR} with File::PATH_SEPARATOR.to_s is wrong and the same applies to the #{sbin} and sbin.to_s pair. A relatively easy way to check this is to enter both regular expressions into brew irb (though we'll need to also provide sbin):

$ brew irb ==> Interactive Homebrew Shell Example commands available with: brew irb --examples irb(main):001:0> sbin = HOMEBREW_PREFIX/"sbin" => #<Pathname:/opt/homebrew/sbin> irb(main):002:0> %r{(?:^|#{File::PATH_SEPARATOR})#{sbin}} => /(?:^|:)\/opt\/homebrew\/sbin/ irb(main):003:0> /(?:^|File::PATH_SEPARATOR.to_s)sbin.to_s/ => /(?:^|File::PATH_SEPARATOR.to_s)sbin.to_s/

The correct substitution should be:

/(?:^|#{Regexp.escape(File::PATH_SEPARATOR)})#{Regexp.escape(sbin)}/

(Regexp.escape makes additionally sure to properly handle any characters that we want to be interpreted literally, but that have special meaning when used inside a regular expression.)

eirinikos · 2016-07-27T20:07:26Z

Sorry -- I accidentally closed this PR when I decided to rename my local and remote branches (without realizing the effect this would have on the PR). 😳 Working on addressing the feedback...

eirinikos · 2016-07-28T19:30:56Z

Sorry if I'm being a bit pedantic here, but “bugs” feels like to strong of a word. There's nothing inherently wrong or broken about the code, it's just that it doesn't fully conform to certain code style guidelines.

Aside from this there's also a minor issue with the commit subject (and PR title). The way I tend to read them is <area of changed code>: <what was changed> and in this spirit a more fitting subject would be tests: fix code style issues or something similar.

No worries, that makes sense to me. I thought I remembered hearing these issues referred to as "bugs" some time ago, but I see that that isn't the most accurate way to refer to them. I've updated the commit and PR title, though the branch still carries the old name. 😬

Thanks for pointing me to the Rubocop documentation on temporarily disabling cops. I've done this for the Method definitions must not be nested and Use snake_case for method names warnings. I followed the example of some existing calls to rubocop:disable and included a code comment explaining the purpose of disabling Rubocop (one comment per type of cop, per file).

I also removed the string interpolation in test_formulary and test_tap since it was unnecessary in both instances (I hadn't actually examined these offenses before).

It's probably obvious that I approached this task with haste and impatience, so I've learned my lesson. Thank you @UniqMartin and @DomT4 for taking a closer look here! 😁

9 Rubocop offenses remain in Homebrew/test; I hope it's OK for these to stay on the back-burner for now:

== test_dependency_collector.rb ==
C: 10: 40: Avoid the use of the case equality operator ===.
== test_ENV.rb ==
W: 31:  5: Do not suppress exceptions.
W: 31:  5: Avoid rescuing the Exception class. Perhaps you meant to rescue StandardError?
== test_formula.rb ==
W:511: 23: Comparison of something with itself detected.
C:523: 29: Do not use semicolons to terminate expressions.
== test_patching.rb ==
C:126:  9: Avoid multi-line chains of blocks.
C:140:  9: Avoid multi-line chains of blocks.
C:239:  9: Avoid multi-line chains of blocks.
== testing_env.rb ==
C: 45:  7: Replace class var @@log with a class instance var.

MikeMcQuaid · 2016-08-02T10:01:37Z

Library/Homebrew/test/test_integration_cmds.rb

@@ -193,7 +193,7 @@ def test_cellar_formula
  end

  def test_env
-    assert_match %r{CMAKE_PREFIX_PATH="#{HOMEBREW_PREFIX}[:"]},
+    assert_match /CMAKE_PREFIX_PATH="#{Regexp.escape(HOMEBREW_PREFIX)}[:"]/,


This needs to be wrapped with (): http://bot.brew.sh/job/Brew%20Pull%20Requests/1111/version=el_capitan/testReport/junit/brew-test-bot/el_capitan/readall___syntax/

Oops. Got it. 👍

...though now Rubocop is saying Don't use parentheses around a literal... 😑

Sorry, needs to be this line and the next

Do you mean like this?

assert_match(/CMAKE_PREFIX_PATH="#{Regexp.escape(HOMEBREW_PREFIX)}[:"]/, cmd("--env"))

MikeMcQuaid · 2016-08-02T10:06:13Z

@eirinikos Nice work! Added a few comments and then we can 🚢 this.

eirinikos · 2016-08-02T18:25:19Z

@MikeMcQuaid Thanks!

MikeMcQuaid · 2016-08-04T10:39:53Z

Library/Homebrew/test/.rubocop.yml

+
+# `formula do` uses nested method definitions
+Style/NestedMethodDefinition:
+  Enabled: false


Add a newline at the end of this file (ideally configure your editor to do this automagically!)

Got it! 👍

UniqMartin · 2016-08-06T05:06:13Z

Merged in 8ec5925. Thank you for another great contribution to Homebrew, @eirinikos! 🎉

MikeMcQuaid · 2016-08-08T09:20:49Z

Nice work @eirinikos 👏

DomT4 reviewed Jul 25, 2016
View reviewed changes

UniqMartin added the gsoc-outreachy Google Summer of Code or Outreachy label Jul 27, 2016

UniqMartin reviewed Jul 27, 2016
View reviewed changes

eirinikos closed this Jul 27, 2016

eirinikos deleted the fix-style-bugs-homebrew-test branch July 27, 2016 19:56

eirinikos restored the fix-style-bugs-homebrew-test branch July 27, 2016 19:57

eirinikos changed the title ~~style: fix style bugs in Homebrew/test~~ tests: fix code style issues Jul 27, 2016

eirinikos reopened this Jul 27, 2016

eirinikos force-pushed the fix-style-bugs-homebrew-test branch from 5f6df5d to 556af40 Compare July 28, 2016 19:03

MikeMcQuaid reviewed Aug 2, 2016
View reviewed changes

eirinikos force-pushed the fix-style-bugs-homebrew-test branch from 556af40 to 914893c Compare August 2, 2016 17:46

MikeMcQuaid reviewed Aug 4, 2016
View reviewed changes

tests: fix code style issues

2a67d70

eirinikos force-pushed the fix-style-bugs-homebrew-test branch from 914893c to 2a67d70 Compare August 4, 2016 19:16

UniqMartin merged commit 8ec5925 into Homebrew:master Aug 6, 2016

Homebrew locked and limited conversation to collaborators May 3, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests: fix code style issues #587

tests: fix code style issues #587

eirinikos commented Jul 25, 2016

DomT4 Jul 25, 2016

eirinikos Jul 25, 2016

UniqMartin commented Jul 27, 2016

UniqMartin Jul 27, 2016

eirinikos commented Jul 27, 2016 •

edited

eirinikos commented Jul 28, 2016 •

edited

MikeMcQuaid Aug 2, 2016

eirinikos Aug 2, 2016

eirinikos Aug 2, 2016

MikeMcQuaid Aug 2, 2016

eirinikos Aug 2, 2016

MikeMcQuaid Aug 2, 2016

MikeMcQuaid commented Aug 2, 2016

eirinikos commented Aug 2, 2016

MikeMcQuaid Aug 4, 2016

eirinikos Aug 4, 2016

UniqMartin commented Aug 6, 2016

MikeMcQuaid commented Aug 8, 2016

tests: fix code style issues #587

tests: fix code style issues #587

Conversation

eirinikos commented Jul 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

UniqMartin commented Jul 27, 2016

Choose a reason for hiding this comment

eirinikos commented Jul 27, 2016 • edited

eirinikos commented Jul 28, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeMcQuaid commented Aug 2, 2016

eirinikos commented Aug 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

UniqMartin commented Aug 6, 2016

MikeMcQuaid commented Aug 8, 2016

eirinikos commented Jul 27, 2016 •

edited

eirinikos commented Jul 28, 2016 •

edited