Skip to content

Don't use Minitest for running tests in a Rails app #773

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 21, 2023
Merged

Don't use Minitest for running tests in a Rails app #773

merged 1 commit into from
Jun 21, 2023

Conversation

@andyw8
Copy link
Contributor

@andyw8 andyw8 commented Jun 21, 2023 •
edited
Loading

Motivation

The testing behaviour provided by Rails is built on top of minitest, but invoked differently. If a repo has a direct dependency on both minitest and rails, then we want the test running to be handled by ruby-lsp-rails instead.

Implementation

Adjust the conditional.

Automated Tests

Included

Manual Tests

On Core, configure:

  gem "ruby-lsp", require: true, github: "Shopify/ruby-lsp", branch: "andyw8/dont-use-minitest-for-test-running-in-rails-app"
  gem "ruby-lsp-rails", require: true, github: "Shopify/ruby-lsp-rails", branch: "andyw8/add-code-lens-extension"

Open a test and verify that the Run in Terminal code lenses for the both the class and the test methods use bin/rails test ... and not ruby -Itest ...

@github-actions
Copy link
Contributor

github-actions bot commented Jun 21, 2023 •
edited
Loading 

Benchmark results in seconds (slowest at top)

          textDocument/completion average: 0.265347 std_dev: 0.004917
          textDocument/diagnostic average: 0.042353 std_dev: 0.009043
      textDocument/selectionRange average: 0.003507 std_dev: 0.000556
   textDocument/documentHighlight average: 0.001827 std_dev: 0.000118
 textDocument/semanticTokens/full average: 0.001772 std_dev: 0.000283
      textDocument/documentSymbol average: 0.001764 std_dev: 8.3e-05
        textDocument/documentLink average: 0.001761 std_dev: 0.000154
            textDocument/codeLens average: 0.001754 std_dev: 0.000136
        textDocument/foldingRange average: 0.001562 std_dev: 0.000106
textDocument/semanticTokens/range average: 0.000899 std_dev: 4.4e-05
               codeAction/resolve average: 0.00065 std_dev: 7.6e-05
           textDocument/inlayHint average: 0.000616 std_dev: 4.0e-05
               textDocument/hover average: 0.000613 std_dev: 8.1e-05
    textDocument/onTypeFormatting average: 0.000119 std_dev: 6.2e-05
          textDocument/formatting average: 6.8e-05 std_dev: 0.000163
          textDocument/codeAction average: 4.5e-05 std_dev: 3.0e-05


================================================================================
Comparison with main branch:

 textDocument/semanticTokens/full unchanged
textDocument/semanticTokens/range unchanged
      textDocument/documentSymbol unchanged
        textDocument/foldingRange unchanged
          textDocument/formatting unchanged
          textDocument/diagnostic unchanged
        textDocument/documentLink unchanged
           textDocument/inlayHint unchanged
      textDocument/selectionRange unchanged
   textDocument/documentHighlight unchanged
               textDocument/hover unchanged
          textDocument/codeAction unchanged
    textDocument/onTypeFormatting unchanged
               codeAction/resolve unchanged
          textDocument/completion unchanged
            textDocument/codeLens unchanged

@andyw8 andyw8 force-pushed the andyw8/dont-use-minitest-for-test-running-in-rails-app branch 2 times, most recently from 5040f8c to b47a6c2 Compare June 21, 2023 15:12
@andyw8 andyw8 changed the title WIP: Don't use Minitest for running tests in a Rails app Don't use Minitest for running tests in a Rails app Jun 21, 2023
@andyw8 andyw8 marked this pull request as ready for review June 21, 2023 15:12
@andyw8 andyw8 requested a review from a team as a code owner June 21, 2023 15:13
@andyw8 andyw8 requested review from Morriar and egiurleo June 21, 2023 15:13
@andyw8 andyw8 added the enhancement New feature or request label Jun 21, 2023
vinistock
vinistock approved these changes Jun 21, 2023
View reviewed changes
lib/ruby_lsp/requests/support/dependency_detector.rb Outdated
def detected_test_library
# A Rails app may have a dependency on minitest, but we would instead want to use the Rails test runner provided
# by ruby-lsp-rails.
return "rails" if direct_dependency?(/^rails$/)
Copy link
Member

@vinistock vinistock Jun 21, 2023

Choose a reason for hiding this comment

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

Let's add this to the if/else chain below to maintain the same style.

@andyw8 andyw8 force-pushed the andyw8/dont-use-minitest-for-test-running-in-rails-app branch from b47a6c2 to d9148a3 Compare June 21, 2023 15:45
@andyw8 andyw8 enabled auto-merge June 21, 2023 15:46
@andyw8 andyw8 merged commit d22f07d into main Jun 21, 2023
@andyw8 andyw8 deleted the andyw8/dont-use-minitest-for-test-running-in-rails-app branch June 21, 2023 15:50
@shopify-shipit shopify-shipit bot temporarily deployed to production June 23, 2023 15:44 Inactive
vinistock pushed a commit that referenced this pull request Feb 28, 2024
@github-actions
Merge pull request #773 from Shopify/dependabot/npm_and_yarn/types/no…
6292cc6 
…de-20.5.9

Bump @types/node from 20.5.7 to 20.5.9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vinistock vinistock vinistock approved these changes

@Morriar Morriar Awaiting requested review from Morriar Morriar was automatically assigned from Shopify/ruby-dev-exp

@egiurleo egiurleo Awaiting requested review from egiurleo egiurleo was automatically assigned from Shopify/ruby-dev-exp

Assignees

No one assigned

Labels

enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@andyw8 @vinistock