Skip to content
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

Fix minitest spec code lens selectors #2706

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Conversation

thomasmarshall
Copy link
Contributor

Motivation

Closes #2648 and #2688.

This PR improves minitest spec code lens in two ways:

  • Fixes the issue I introduced (sorry!) in Fix minitest code lens filter patterns #2522 whereby the selectors for minitest specs inside classes were incorrect, meaning the commands matched no tests either in the explorer or terminal.
  • Changes the base command to add spec to the load path instead of test if the current path is inside a spec directory (for example tapioca).

Note: This doesn't fix the issue with the code lens above the class in spec files not matching any tests. That has been broken since before my earlier changes and is more tricky to fix because when we generate the code lens we don't yet know if we're in a class that wraps minitest/spec examples or not.

Implementation

To fix the specs inside classes issue, I started tracking the group type (:class, :module, :describe) alongside the group name in @group_stack. It then filters out anything except the :describe groups when constructing the selector.

To fix the load path option, it checks to see whether the current path is inside a spec directory or a test directory and adds an appropriate -I argument to the base command. This changes the default behavior—if the current path is in neither a spec or test directory it no longer adds -Itest.

There are of course weird edge cases here, for example if the path is inside both a spec and test directory (like test/spec/foo_test.rb). In that case both test and spec would be added to the load path, but I don't currently see a better way of guessing which directory should be in the load path.

Automated Tests

I added a new test case to capture the original behavior for minitest spec code lens' and then updated it to match each fix. I also regenerated the expectation JSON files, which have mostly changed just to remove -Itest. I guess we could change the logic so -Itest is added by default, even if it's not in a test directory, and only switch to -Ispec if necessary. That would minimize the expectation JSON changes here.

Manual Tests

Use code lens to run specs in the tapioca codebase:

spec-code-lens.mov

Use code lens to make sure non-spec tests still work fine:

test-code-lens.mov

This commit adds a test case capturing the current behavior of code lens
generation for minitest/spec tests. Although the behavior is actually
incorrect, it's useful to have a test case capturing how it currently
works. In later commits, we'll fix the behavior and update this test.
Minitest specs don't include the class name in the test selector, so we
need to strip out the class name (including the module nesting) when we
generate the selector.

This commit changes `@group_stack` to be a tuple of `[type, value]` so
we can distinguish between class/module names and test group/example
names. For specs, it removes the class/module names from the stack.
Some codebases (particularly those using minitest/spec) have tests in
the `spec` directory instead of the `test` directory. This commit adds
support for detecting these paths and adjusting the load path
accordingly.
@thomasmarshall thomasmarshall requested a review from a team as a code owner October 10, 2024 18:40
@thomasmarshall thomasmarshall marked this pull request as draft October 10, 2024 22:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Run in terminal command above class name is not running all tests properly
1 participant