Skip to content
This repository has been archived by the owner on May 30, 2024. It is now read-only.

Fix Run Current Test In Terminal for class line #961

Merged
merged 1 commit into from
Jan 5, 2024

Conversation

snutij
Copy link
Contributor

@snutij snutij commented Jan 4, 2024

Motivation

Closes: Shopify/ruby-lsp#1743

Implementation

Previously, given this code

class Foo # cursor here
 
  def test_one
    assert(true)
  end

end

The test item called 'Foo' has children called 'test_one'. We set the 'testItem' to run with the value of the recursive method findTestByActiveLine. But in this case this will be undefined, as the cursor is not in range of test_one. And as this is an else_if condition, we do not go through the assignation latter.

I just reworked condition:

  • We assign the testItem with this Foo test.
  • If there is a child, we call the method again with children subset
    • if a child match the range, we assign the value (between Foo and test_one, both can match the cursor range, but child have priority)
    • if no child match the range, we do not assign undefined, like that we keep the first assignation

Automated Tests

There are no automated tests for TestController, do you think it's worth it to avoid this kind of regression?

Manual Tests

Checkout this branch, and run tests with command with cursor at the top level of group (on class level for example)

record

Check the command output, first, test_one level, is suffixed by :7, others are the top level command.

Jan-04-2024 20-31-21

@snutij snutij requested a review from a team as a code owner January 4, 2024 19:40
test.uri?.toString() === editor.document.uri.toString() &&
test.range?.start.line! <= line &&
test.range?.end.line! >= line
) {
testItem = test;
}

if (test.children.size > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't this be an else if? Am I misunderstanding the conditional on line 403?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is not possible, for example:

class Foo 
 
  def test_one # cursor here
    assert(true)
  end

end

The first condition will match (parent test range match the cursor) but you want to override the testItem value with child. Is it clearer?

Copy link
Member

@vinistock vinistock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Thanks for fixing this.

We should definitely add tests for the TestController, but we don't have to block this PR for that

@vinistock vinistock merged commit 91d8a61 into Shopify:main Jan 5, 2024
6 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Run Current Test In Terminal fails for class
3 participants