Don't generate test code lenses when the test library is not supported #769
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…d/unknown Since we want extensions to provide code lenses for libraries like RSpec, we should not generate any code lens ourselves when the test library is not supported/unknown. Otherwise it could create wrong code lenses and mess with with what our extensions generate.
st0012
commented
Jun 20, 2023
| sig { params(node: SyntaxTree::Node, name: String, command: String, kind: Symbol).void } | ||
| def add_test_code_lens(node, name:, command:, kind:) | ||
| # don't add code lenses if the test library is not supported or unknown | ||
| return unless SUPPORTED_TEST_LIBRARIES.include?(@test_library) |
Member
Author
There was a problem hiding this comment.
Just an idea: if we split test code lens listeners into 2, like TestCodeLens and BundlerCodeLens, we can skip registering events in TestCodeLens if the test library is not supported. I think this will save us some unnecessary computation.
If it's preferred, now could be a good time for this before we ship ruby-lsp-rails' code lens support.
WDYT? @vinistock @andyw8
Contributor
There was a problem hiding this comment.
I think it's not critical for now though, we can bump the ruby-lsp-rails dependency later if we refactor things.
Member
There was a problem hiding this comment.
I don't think splitting is worth the complexity yet.
bitwise-aiden
approved these changes
Jun 20, 2023
Contributor
|
andyw8
approved these changes
Jun 20, 2023
vinistock
reviewed
Jun 20, 2023
| sig { params(node: SyntaxTree::Node, name: String, command: String, kind: Symbol).void } | ||
| def add_test_code_lens(node, name:, command:, kind:) | ||
| # don't add code lenses if the test library is not supported or unknown | ||
| return unless SUPPORTED_TEST_LIBRARIES.include?(@test_library) |
Member
There was a problem hiding this comment.
I don't think splitting is worth the complexity yet.
Comment on lines
+77
to
+79
| response = listener.response | ||
|
|
||
| assert_equal(0, response.size) |
Member
There was a problem hiding this comment.
Suggested change
| response = listener.response | |
| assert_equal(0, response.size) | |
| assert_empty(listener.response) |
Comment on lines
+59
to
+61
| response = listener.response | ||
|
|
||
| assert_equal(0, response.size) |
Member
There was a problem hiding this comment.
Suggested change
| response = listener.response | |
| assert_equal(0, response.size) | |
| assert_empty(listener.response) |
| assert_equal(0, response.size) | ||
| end | ||
|
|
||
| def test_no_code_lens_for_rspec |
Member
There was a problem hiding this comment.
Does this test verify any behaviour that the unknown one doesn't? Do we need both?
vinistock
pushed a commit
that referenced
this pull request
Feb 28, 2024
…de-20.5.7 Bump @types/node from 20.5.1 to 20.5.7
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Since we want extensions to provide code lenses for libraries like RSpec, we should not generate any code lens ourselves when the test library is not supported/unknown. Otherwise it could create wrong code lenses and mess with with what our extensions generate.
Implementation
minitest, we simply returnunknownif it's not a known one.Automated Tests
Added 2 test cases to make sure code lenses will not be generated when the library is
rspecorunknown.Manual Tests