Fix CI to be green #780
Fix CI to be green #780
Conversation
|
These all look good to me! @rafaelfranca |
fix options spec. allow line_editor spec to be run independently running `rspec spec/line_editor_spec.rb` generated a double error when it tries to re require "readline" fix expectations for ruby 3 treatment of hash arg try coveralls_reborn to fix ssl errors. Note that we could also use the coveralls action as recommended in https://github.com/tagliala/coveralls-ruby-reborn but it seems like a github token is needed, which makes it more complex for contributors This does mean dropping coveralls for EOLed rubies but do we really need to post to coveralls on each test run? Wouldn't one test run be enough?
| expect { check_unknown! }.to raise_error(Thor::UnknownArgumentError, expected) | ||
| expect { check_unknown! }.to raise_error(Thor::UnknownArgumentError) do |error| | ||
| expect(error.to_s).to eq(expected) | ||
| end |
There was a problem hiding this comment.
I wonder, shouldn't rather be the
Thor::Correctableline above dropped instead? But that would probably not be backward compatible with older rspec-mock 🤔
I can validate that removing that line instead does fix it for ruby 3.0.
This is caused by rspec/rspec-expectations#1339 just for the context.
But I don't think this is the same issue addressed by that PR, because rspec/rspec-expectations#1339 was included in rspec-expectations v3.11.0, and using that version, you still get an error in main.
I'm neutral on which way to fix it (just want the CI to be green 😀 )
There was a problem hiding this comment.
I think that the rspec-expectations PR now tries to preserve the original exception message, which is not modified by error_higlight or did you mean. And that is the reason why the test case fails.
Actually, thinking about it once again, the error failure reported by RSpec might be still misleading, because it says something different then it does. This is the message:
expected Thor::UnknownArgumentError with "Unknown switches \"--baz\"\nDid you mean? \"--bar\"", got #<Thor::UnknownArgumentError: Unknown switches "--baz"
Did you mean? "--bar"> with backtrace:
... snip ...
But because it internally compares the expected message with the original message, is should actually report:
expected Thor::UnknownArgumentError with "Unknown switches \"--baz\"\nDid you mean? \"--bar\"", got #<Thor::UnknownArgumentError: Unknown switches "--baz"> with backtrace:
Because that is what is actually happening on background.
There was a problem hiding this comment.
Reported as rspec/rspec-expectations#1362
There was a problem hiding this comment.
@voxik can you clarify for me - is there any action needed on this PR or is it good to go in your opinion?
There was a problem hiding this comment.
That is really not up to me. I just randomly hit the similar issue and tried to understand the code and provide some feedback. It is ultimately up to Thor folks to decide what this test should actually test. If the DidYouMean / Thor::Correctable should be covered by this test case or not. I think that it should be extracted into independent test case, but I'll be also fine if the test suite is green 🟢
|
@dorner Anything I can do to help get this merged? |
|
@rafaelfranca ping? |
lib/thor/line_editor/readline.rb
Outdated
| class Thor | ||
| module LineEditor | ||
| class Readline < Basic | ||
| def self.available? | ||
| begin | ||
| require "readline" |
There was a problem hiding this comment.
@deivid-rodriguez wasn't this added because of bundler? if we move out to the top of the file would not that mean that bundler will require readline too early?
There was a problem hiding this comment.
This change isn't necessary to get CI green (it was only so I could run one spec independently). If it's blocking the merge I can just remove this commit from the PR.
There was a problem hiding this comment.
Yes, I added this because of Bundler. I don't recall the specifics, but it was something related to Windows I believe. I'd prefer if this commit was removed, yes.
I did reproduce the failing spec in isolation, but that can be fixed in the spec itself without giving up on requiring deadline lazily:
diff --git a/spec/line_editor_spec.rb b/spec/line_editor_spec.rb
index f034ec8..9c6250e 100644
--- a/spec/line_editor_spec.rb
+++ b/spec/line_editor_spec.rb
@@ -1,8 +1,9 @@
require "helper"
+require "readline"
describe Thor::LineEditor, "on a system with Readline support" do
before do
- @original_readline = ::Readline if defined? ::Readline
+ @original_readline = ::Readline
silence_warnings { ::Readline = double("Readline") }
end
@@ -22,10 +23,8 @@ end
describe Thor::LineEditor, "on a system without Readline support" do
before do
- if defined? ::Readline
- @original_readline = ::Readline
- Object.send(:remove_const, :Readline)
- end
+ @original_readline = ::Readline
+ Object.send(:remove_const, :Readline)
end
after doThere was a problem hiding this comment.
@deivid-rodriguez I've dropped the commit for now (can bring back running the spec on its own but that's a nice to have), but we've now got two failing specs in ruby head (unrelated to this change). I don't see an immediate fix for this, and haven't got loads of time to research right now.
There was a problem hiding this comment.
Is the failure what's fixed by #771?
There was a problem hiding this comment.
Ah, ok! In that case I think it makes sense to merge this, and I can work on the fix for the ruby-head issue separately.
There was a problem hiding this comment.
Ruby Head should be fixed by #789!
There was a problem hiding this comment.
Also fixed running line editor specs in isolation at #791!
Note that we could also use the coveralls action as recommended in https://github.com/tagliala/coveralls-ruby-reborn but it seems like a github token is needed, which makes it more complex for contributors This does mean dropping coveralls for EOLed rubies but do we really need to post to coveralls on each test run? Wouldn't one test run be enough?
timdiggins
force-pushed
the
fix-ci-integration
branch
from
5ce3ae6
to
4ee68cb
Compare
🌈 This is a combination of #777, #778, #779 if you prefer a single green PR 😀