RuboCopBear: Fix CI breakage #1890

yash-nisar · 2017-07-05T08:51:18Z

rubocop/rubocop#4226 indicates that --stdin
takes filename as an argument which was incorporated. Also, the tests
were enhanced that involved the bear to be run on files with the .rb
(ruby) extension because initially the bear was run on the generated
temporary files that was the cause of error.

Fixes #1548

For short term contributors: we understand that getting your commits well
defined like we require is a hard task and takes some learning. If you
look to help without wanting to contribute long term there's no need
for you to learn this. Just drop us a message and we'll take care of brushing
up your stuff for merge!

Checklist

I read the commit guidelines and I've followed
them.
I ran coala over my code locally. (All commits have to pass
individually. It is not sufficient to have "fixup commits" on your PR,
our bot will still report the issues for the previous commit.) You will
likely receive a lot of bot comments and build failures if coala does not
pass on every single commit!

After you submit your pull request, DO NOT click the 'Update Branch' button.
When asked for a rebase, consult coala.io/rebase
instead.

Please consider helping us by reviewing other peoples pull requests as well:

pick up any PR at https://coala.io/review
review it (check https://coala.io/reviewing for more info)
if you are sure that it needs work, use cobot mark wip <URL> to get it out
of the review queue.

The more you review, the more your score will grow at coala.io and we will
review your PRs faster!

gitmate-bot · 2017-07-05T08:52:15Z

tests/ruby/RuboCopBearTest.py

-                                    invalid_files=(bad_file,),
-                                    valid_files=(good_file,))
+@generate_skip_decorator(RuboCopBear)
+class RuboCopBearTest(LocalBearTestHelper):


The code does not comply to PEP8.

PEP8Bear, severity NORMAL, section autopep8.

The issue can be fixed by applying the following patch:

--- a/tests/ruby/RuboCopBearTest.py +++ b/tests/ruby/RuboCopBearTest.py @@ -23,6 +23,7 @@ @generate_skip_decorator(RuboCopBear) class RuboCopBearTest(LocalBearTestHelper): + def setUp(self): self.uut = RuboCopBear(Section('name'), Queue())

jayvdb · 2017-07-05T09:06:10Z

bears/ruby/RuboCopBear.py

@@ -36,7 +36,7 @@ class RuboCopBear:
    def create_arguments(filename, file, config_file, rubocop_config: str=''):
        # Need both stdin and filename. Explained in this comment:
        # https://github.com/bbatsov/rubocop/pull/2146#issuecomment-131403694
-        args = (filename, '--stdin', '--format=json')
+        args = ('--stdin', filename, '--format=json')


Is this change compatible with earlier versions of rubocop?

Yes 👍 , tested will versions 0.47.1, 0.48.1, 0.49.1

jayvdb · 2017-07-05T09:47:50Z

tests/ruby/RuboCopBearTest.py


-good_file = """def good_name


why do we need to move the text out into real filenames?

Does rubocop require that there is a valid file with this filename?

Cant LocalBearTestHelper create temporary files with an appropriate filename/extension?

If it cant, then writing tests is much harder, which is sort of the anti-thesis of your project.

Yes, rubocop requires that we have a valid filename extension (.rb). Temporary files were created in the previous case when we had to use verify_local_bear(...) but without a valid extension. See the commit message.

So, you need to improve LocalBearTestHelper :P

Actually, this is a linter specific problem. This is the first linter that I've come across that is careful about the filename extension. Moreover, it would make less sense IMO to improve LocalBearTestHelper to take care of this case only. Also we had to somehow improve the tests and use check_results(...) instead of verify_local_bear(...).

yash-nisar · 2017-07-05T14:49:12Z

@Makman2 Tagging you since this PR involved improvisation of tests.

Makman2 · 2017-07-05T18:59:59Z

bears/ruby/RuboCopBear.py

@@ -19,7 +19,7 @@ class RuboCopBear:
    """

    LANGUAGES = {'Ruby'}
-    REQUIREMENTS = {GemRequirement('rubocop', '0.47.1'),
+    REQUIREMENTS = {GemRequirement('rubocop'),


why did you remove the version specification?

Our motive is to use the latest version of rubocop. We were using 0.47.1 in the earlier case and due to an upstream release i.e. 0.48, our CI broke. As a temporary fix, I had to pin the version to 0.47.1 to make sure that the tree was green again. Now that we have a solution, I think it would be a good option to get rid of the pinning and install the latest version.

though I would recommend to fix it currently for 0.48, and upgrade to newer releases explicitly with an issue. But up to you 👍 Not sure actually what our requirements strategy is for bears^^

Makman2 · 2017-07-05T19:03:47Z

tests/ruby/RuboCopBearTest.py

+                                end_line=2,
+                                end_column=20,
+                                severity=RESULT_SEVERITY.INFO)],
+            filename=get_testfile_path(filename),


you should also check good files 👍

Makman2 · 2017-07-05T19:04:35Z

You could write into the commit message body that part of the fixing was to improve the tests.

yash-nisar · 2017-07-06T07:39:47Z

Updated @Makman2

Makman2 · 2017-07-06T19:30:38Z

tests/ruby/RuboCopBearTest.py

+            settings={'indent_size': 1})
+
+    def test_good_indent_size(self):
+        filename = 'bad_indent_file.rb'


maybe just call the file indent.rb, as in this context it isn't bad?

Makman2 · 2017-07-06T19:56:15Z

ack 51507c5

rubocop/rubocop#4226 indicates that `--stdin` takes filename as an argument which was incorporated. A part of the fixing was to improve the tests that involved the bear to be run on files with the `.rb`(ruby) extension because initially the bear was run on the generated temporary files that was the cause of error. Fixes coala#1548

Makman2 · 2017-07-06T20:37:40Z

reack d35b120

Makman2 · 2017-07-06T20:38:36Z

@rultor merge

rultor · 2017-07-06T20:39:03Z

@rultor merge

@Makman2 OK, I'll try to merge now. You can check the progress of the merge here

rultor · 2017-07-06T20:40:49Z

@rultor merge

@Makman2 Done! FYI, the full log is here (took me 2min)

gitmate-bot added process/pending review size/L review carefully! labels Jul 5, 2017

gitmate-bot reviewed Jul 5, 2017

View reviewed changes

yash-nisar force-pushed the rubocop-investigation branch from 8794304 to 97d1d70 Compare July 5, 2017 08:53

jayvdb reviewed Jul 5, 2017

View reviewed changes

Makman2 reviewed Jul 5, 2017

View reviewed changes

yash-nisar force-pushed the rubocop-investigation branch 3 times, most recently from e1139dc to 2359913 Compare July 6, 2017 07:39

Makman2 reviewed Jul 6, 2017

View reviewed changes

yash-nisar force-pushed the rubocop-investigation branch from 2359913 to 51507c5 Compare July 6, 2017 19:39

Makman2 added process/approved and removed process/pending review labels Jul 6, 2017

yash-nisar force-pushed the rubocop-investigation branch from 51507c5 to d35b120 Compare July 6, 2017 19:57

gitmate-bot added the process/pending review label Jul 6, 2017

Makman2 removed the process/pending review label Jul 6, 2017

rultor merged commit d35b120 into coala:master Jul 6, 2017

yash-nisar deleted the rubocop-investigation branch July 6, 2017 20:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RuboCopBear: Fix CI breakage #1890

RuboCopBear: Fix CI breakage #1890

yash-nisar commented Jul 5, 2017 •

edited

Loading

gitmate-bot Jul 5, 2017

jayvdb Jul 5, 2017

yash-nisar Jul 5, 2017

jayvdb Jul 5, 2017

yash-nisar Jul 5, 2017

jayvdb Jul 5, 2017

yash-nisar Jul 5, 2017

yash-nisar commented Jul 5, 2017

Makman2 Jul 5, 2017

yash-nisar Jul 6, 2017 •

edited

Loading

Makman2 Jul 6, 2017

Makman2 Jul 5, 2017

yash-nisar Jul 6, 2017

Makman2 commented Jul 5, 2017

yash-nisar commented Jul 6, 2017

Makman2 Jul 6, 2017

Makman2 commented Jul 6, 2017

Makman2 commented Jul 6, 2017

Makman2 commented Jul 6, 2017

rultor commented Jul 6, 2017

rultor commented Jul 6, 2017

RuboCopBear: Fix CI breakage #1890

RuboCopBear: Fix CI breakage #1890

Conversation

yash-nisar commented Jul 5, 2017 • edited Loading

Checklist

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

Choose a reason for hiding this comment

yash-nisar commented Jul 5, 2017

Choose a reason for hiding this comment

yash-nisar Jul 6, 2017 • edited Loading

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

Makman2 commented Jul 5, 2017

yash-nisar commented Jul 6, 2017

Choose a reason for hiding this comment

Makman2 commented Jul 6, 2017

Makman2 commented Jul 6, 2017

Makman2 commented Jul 6, 2017

rultor commented Jul 6, 2017

rultor commented Jul 6, 2017

yash-nisar commented Jul 5, 2017 •

edited

Loading

yash-nisar Jul 6, 2017 •

edited

Loading