-
Notifications
You must be signed in to change notification settings - Fork 174
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
Move CI command checks into testsuite #2609
Conversation
77dff24
to
6fe4574
Compare
5ca33b5
to
7b70dc9
Compare
Thank you for the PR! I put #2610 up to address the issue quickly. However, as pointed out in #2610 (comment), we should have an integration test to ensure this doesn't happen again. |
Okay, the fix itself is merged. We'd love to have that integration test shipped. |
The CI commands use the Gemfile of the repo, which contains additional gems like `rubocop` and `syntax_tree`. This hides issues like Shopify#2604. So, create a separate Gemfile containing only ruby-lsp and use that instead
7b70dc9
to
014944e
Compare
Alright, rebased and left only the tests in. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the contribution!
Motivation
Closes #2604
Alternative to #2605
Implementation
Just move all the rubocop-specific requires into their own file and gate it once there.
Automated Tests
This moves the two CI checks into the test suite. They would have caught this error, expect rubocop is available to these. So, create a seperate Gemfile that contains only the gem itself. I still have them only run in CI now, they take a bit of time (though not that much overall)
Manual Tests
I tested that ruby-lsp works with and without rubocop in a users gemfile. Offenses show up as expected.