-
Notifications
You must be signed in to change notification settings - Fork 153
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
Tests: Use YARP fixtures #788
Tests: Use YARP fixtures #788
Conversation
This looks great! Some of the failures might take a while to address, so it could be useful to have a way to mark those as known failures, which can be fixed incrementally in future. |
This is not ideal, but allow us to go forward, and already provide a lot more fixture. wdyt? |
That seems reasonable. I haven't looked in detail at the failures yet, are they all from RuboCop? I would have thought we might see some caused by bugs in ruby-lsp itself. Btw, one thing I noticed when trying this locally: Initially everything was wrongly passing, because I hadn't run yet |
Also, @kddnewton suggested using https://github.com/ruby/yarp/tree/main/test/fixtures which is a larger set which includes all the syntax tree fixtures, but that could be done later. |
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.
This is awesome and already showing value given that it caught many errors
👋 After some more internal discussion:
Thanks again for your contribution! |
Thanks for coming back @andyw8. 🙏 For the first point, YARP fixtures are not a repository (but only a subfolder from a repository). If we want to use it as a submodule, we have to point to the full git project (and pull the full YARP repo). It's look not ideal to me, maybe I'm missing your point? Perhaps there is a way to do that, that I'm not aware of yet. 😄 For the rest of your comment, I'll do it 👍 |
Yes we're ok with pulling the full YARP repo, it's fairly small. (@kddnewton said he may publish the fixtures to a separate repo at some point). |
1ed6c86
to
2d78109
Compare
I moved from I'll dig asap on these errors to know which are on Please feel free to give feedback on the expectation_test_runner. I tried to make it simple, but I'm not sure if it worked. 😄 |
1f7edb0
to
8840148
Compare
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.
There seems to be at least two failures that are also related to infinite loops in RuboCop when running formatting.
I think we should just use the same technique and ignore any RuboCop related errors by rescuing RubyLsp::Requests::Formatting::Error
.
81c0b6f
to
f99c8e4
Compare
@vinistock thanks for your feedbacks 🙏
|
f99c8e4
to
b69ab63
Compare
Well, CI is green 🎉 feel free to review it! Do you want a refresh on PR description? This is not any more exact, as we move on YARP fixture. |
b69ab63
to
360c641
Compare
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.
This is amazing! Thank you very much for your work on this.
We'll need to wait for a Syntax Tree release with the fix, but it looks good to me.
360c641
to
a7fa90b
Compare
a7fa90b
to
5e691b9
Compare
5e691b9
to
09f1514
Compare
09f1514
to
478c775
Compare
478c775
to
a7a3692
Compare
04c9eba
to
93ecd46
Compare
93ecd46
to
6d4f57a
Compare
Let's not wait for RuboCop to fix stuff. We should start benefiting from the fixtures right away. In RuboCop Runner, let's include That should get the CI passing and we can merge. |
6d4f57a
to
e8ca109
Compare
e8ca109
to
f030e5c
Compare
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 very much for pushing this over the finish line, I know it took a while.
Amazing contribution 🚀
I wonder if we should update the Dependabot config to periodically pull in the submodule updates, it's supported as a |
@andyw8 yep we totally can, it was part of the first iteration based on |
Motivation
Closes: #448
Started with syntax-tree fixtures we decided during this PR to move forward and use YARP fixtures, which is a larger set.
Implementation
bin/test
that the submodule is well initialized.expectations_test_runner.rb
:Automated Tests
More than 14k tests now, all should be green or skipped with "Fixture requires a fix from Rubocop" message.
Manual Tests
git submodule update --init --recursive
bin/test
Then to have a complete stack trace of Rubocop fail:
bin/rubocop -d path/for/fixture