Skip to content

Add test discovery documentation for test addons #3488

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

vinistock
Copy link
Member

@vinistock vinistock commented May 14, 2025

Motivation

Step towards #3421

This PR adds documentation for the test discovery process that add-ons can contribute. I decided to not put command resolution and custom reporters in the same PR because it would make it too long to review.

I also made this a dedicated file under add-ons. It's complex enough that it warrants longer examples with more detailed explanations.

Copy link
Member Author


How to use the Graphite Merge Queue

Add the label graphite-merge to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@vinistock vinistock requested review from alexcrocha and st0012 May 14, 2025 14:33
@vinistock vinistock self-assigned this May 14, 2025
@vinistock vinistock added the documentation Improvements or additions to documentation label May 14, 2025 — with Graphite App
@vinistock vinistock marked this pull request as ready for review May 14, 2025 14:34
@vinistock vinistock requested a review from a team as a code owner May 14, 2025 14:34
@vinistock vinistock force-pushed the 05-14-add_test_discovery_documentation_for_test_addons branch from 05af426 to 17855b5 Compare May 14, 2025 14:35
Copy link
Contributor

@alexcrocha alexcrocha left a comment

Choose a reason for hiding this comment

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

Looks good 💪
Added a couple suggestions and a few nits.


The Ruby LSP's test explorer includes built-in support for Minitest and Test Unit. Add-ons can add support for other
test frameworks, like [Active Support test case](rails-add-on) and [RSpec](https://github.com/st0012/ruby-lsp-rspec).
There are 3 main parts for contributing support for a new framework, which will be discussed in separate sections.
Copy link
Contributor

Choose a reason for hiding this comment

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

We should list what the 3 parts are, and update with links when they're available.

Suggested change
There are 3 main parts for contributing support for a new framework, which will be discussed in separate sections.
There are 3 main parts for contributing support for a new framework:
- [Test discovery](#test-discovery): Identifying tests within the codebase and their structure.
- Command resolution: Determining how to execute a specific test or group of tests.
- Custom reporting: Displaying test execution results in the Test Explorer.```


## Test discovery

Test discovery is the process of populating the explorer view with which tests exist in the codebase. The Ruby LSP
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Test discovery is the process of populating the explorer view with which tests exist in the codebase. The Ruby LSP
Test discovery is the process of populating the explorer view with the tests that exist in the codebase. The Ruby LSP

nit

end
```

Now, we have to implement the listener. If the test framework being handled uses classes to define test groups, like
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Now, we have to implement the listener. If the test framework being handled uses classes to define test groups, like
Next, the listener itself needs to be implemented. If the test framework being handled uses classes to define test groups, like

def on_class_node_enter(node)
# Here we use the `with_test_ancestor_tracking` so that we can check if the class we just found inherits
# from our framework's parent test class. This check is important because users can define any classes or
# modules inside a test file and they are not all runnable tests
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# modules inside a test file and they are not all runnable tests
# modules inside a test file and not all of them are runnable tests

nit

Comment on lines +148 to +149
# Create the test item for the example. To make IDs unique, always include the group names as part of it since
# users can define the same exact example name in multiple different groups
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Create the test item for the example. To make IDs unique, always include the group names as part of it since
# users can define the same exact example name in multiple different groups
# Create the test item for the example. To make IDs unique, always include the group names as part of the ID,
# since users can define the same exact example name in multiple different groups

nit

end
```

There's an implicit requirement for test item IDs. Groups must be separated by `::` and examples must be separated
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
There's an implicit requirement for test item IDs. Groups must be separated by `::` and examples must be separated
Test item IDs have an implicit formatting requirement: Groups must be separated by `::` and examples must be separated

Comment on lines +180 to +182
For frameworks that do not define test groups using classes, such as RSpec, the listener should not inherit from
`RubyLsp::Listeners::TestDiscovery` and the whole logic can be implemented directly based on the specific rules for
that tool.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
For frameworks that do not define test groups using classes, such as RSpec, the listener should not inherit from
`RubyLsp::Listeners::TestDiscovery` and the whole logic can be implemented directly based on the specific rules for
that tool.
For frameworks that do not define test groups using classes, such as RSpec, the listener should not inherit from
`RubyLsp::Listeners::TestDiscovery`. Instead, the logic can be implemented directly, based on the framework's specific
rules.

end
```

There's an implicit requirement for test item IDs. Groups must be separated by `::` and examples must be separated
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Is this implicit ID requirement necesary just for the listeners inheriting from RubyLsp::Listeners::TestDiscovery or is for all the addons?.
  2. Is for example MyTest::some group#test something valid?, Or it has to be underscored without spaces. A regex could be usefull to know the exact required syntax. Because spec like styles do not conform so well to the Module::Class#method syntax.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants