Fix minitest code lens filter patterns #2522
Merged
Conversation
This file contains 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
thomasmarshall
commented
Sep 4, 2024
thomasmarshall
commented
Sep 4, 2024
test/expectations/code_lens/minitest_with_dynamic_constant_path.exp.json
Show resolved
Hide resolved
thomasmarshall
commented
Sep 4, 2024
vinistock
added
bugfix
This commit adds a `WRITE_EXPECTATIONS` environment variable that allows expectation snapshots to be automatically updated when running tests. This is useful when making a change that affects many expectations which would be painful to update manually. However, it should be used with caution as it makes it easy to update expectations without really thinking about it.
This commit adds a test to capture the current behavior of the minitest spec code lenses. These are currently problematic because we sometimes generate the same ID and command for different tests. Later commits will fix both issues, but it would be good to have a test to see how the output changes.
This commit ensures code lenses for minitest/spec tests have more
precise filter patterns so only the selected test is executed. In the
previous implementation, the filter pattern was too broad and would
potentially match multiple tests.
It works by generating a fully qualified selector for the test, matching
the format that is used internally by minitest. It increments a counter
for each group and prefixes the test name with the counter to ensure
uniqueness. This is potentially brittle as it makes an assumption about
the internal implementation of minitest, but it's the best we can do to
ensure only the selected test is run.
It's reasonably common to have multiple tests with the same name, for
example:
```
describe Foo do
describe "#bar" do
it "returns true"
end
describe "#baz" do
it "returns true"
end
end
```
Previously, clicking "Run In Terminal" for the `works` test under `#bar`
would run minitest with the following filter pattern:
> --name /works/
This would match multiple tests and run them both. With this change, the
filter pattern is more specific:
> --name "/^Foo#bar#test_0001_works$/"
This commit ensures the IDs used for the code lenses are unique as far
as possible. This fixes an issue whereby multiple tests could have the
same ID and trying to run one of them would run a different one instead.
This could happen in a number of scenarios. For example, if you have
multiple test classes in the same file:
```
class FooTest < Minitest::Test
def test_foo
end
end
class BarTest < Minitest::Test
def test_foo
end
end
```
Or if you have nested specs:
```
describe Foo do
describe "#bar" do
it "returns true"
end
describe "#baz" do
it "returns true"
end
end
```
In both cases, the code lenses for the tests would have the same ID.
This commit fixes the issue by using a fully qualified selector for each
test, including the group stack and method name. This should ensure that
the IDs are unique.
For example, instead of `test_foo`, the ID would be `FooTest#test_foo`.
And instead of `returns true`, the ID would be `Foo#bar#returns true`.
The only cases that are not covered are dynamic module references which
can't be resolved statically. There is still a chance that two tests
have the same ID:
```
module abc::Foo
class SomeTest < Minitest::Test
def test_foo
end
end
end
module def::Foo
class SomeTest < Minitest::Test
def test_foo
end
end
end
```
Here both IDs would be `<dynamic_reference>::SomeTest#test_foo`. I'm not
sure how important it is to support this case—I expect it's quite rare.
One option would be to increment a counter and append it to the ID
somehow, but the added complexity doesn't seem worth it.
I'd prefer to replace the `name` argument with `id` and `label`, but
that would be a breaking change for plugins that are currently
referencing `add_test_code_lens`. As such, I added `id` as an optional
argument and handle it in the method. I've also kept backwards
compatibility between the extension and older versions of the LSP which
would not have the `id` field.
thomasmarshall
force-pushed
the
fix-code-lens-selectors
branch
from
01850ae
to
9f8f611
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
I'd like to revive #2230 but I noticed there are a couple of small issues with the test code lenses that should be fixed before resuming work:
These two issues can both happen at the same time, for example:
Implementation
To fix the issue with broad filter patterns for minitests specs, I used a more specific selector based on the format minitest uses internally. It uses the
@group_stackinstance variable to capture the path leading to each example, and adds a new@spec_idvariable that tracks how many examples are in each group. This information can be combined into a fully qualified selector likea::b#test_0001_works.To fix the issue with non-unique IDs, I added a
generate_fully_qualified_idmethod that does something similar to above. Instead of using just the method name, the ID will include the name of the group that it sits within.I tried to make this work in a backwards compatible way. I would have preferred to remove
nameand replace it withidandlabelbut plugins are currently using the (private)add_test_code_lensmethod. I also considered backwards compatibility between the extension and old versions of the LSP by making the newlabelargument optional.Automated Tests
I have added an expectation specifically to capture the issue with minitest specs. The other issue was already covered implicitly by various expectation tests. I also added a mechanism to update expectation snapshots because this PR involved many small changes—I can remove that if it's unwanted.
Manual Tests
See the code example above which covers both issues. Try clicking "Run" and "Run In Terminal" for the examples in that test.