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

Add test for lsp_add_on flag in CLI #2101

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

andyw8
Copy link
Contributor

@andyw8 andyw8 commented Nov 28, 2024

Motivation

Part of https://github.com/Shopify/team-ruby-dx/issues/1327

Implementation

Until now, most of Tapioca tests have been integration tests. Trying to verify the new LSP behaviour using this approach will be difficult, so I suggest we should consider a more mockist-style approach.

I'm choosing to use Mocha as it's the standard for Shopify, and it allows a lot flexibility.

I intend to add some integration tests later to verify the LSP behaviour as a whole

TODO:

  • I need to fix the typecheck, but wanted to open this up early for discussion.

Tests

Indeed.

@andyw8 andyw8 added the chore label Nov 28, 2024
@andyw8 andyw8 changed the title Add test for lsp_add-on flag in CLI Add test for lsp_add_on flag in CLI Nov 28, 2024
@andyw8 andyw8 marked this pull request as ready for review November 28, 2024 17:58
@andyw8 andyw8 requested a review from a team as a code owner November 28, 2024 17:58
@andyw8 andyw8 force-pushed the andyw8/add-test-for-lsp-addon-flag-in-cli branch from f665339 to dd9aaad Compare November 28, 2024 17:59
@andyw8 andyw8 force-pushed the andyw8/add-test-for-lsp-addon-flag-in-cli branch from dd9aaad to ffc509c Compare November 28, 2024 18:46
@andyw8
Copy link
Contributor Author

andyw8 commented Nov 28, 2024

(I can split off the Mocha setup into a seperate PR if desired)

spec/tapioca/cli_spec.rb Outdated Show resolved Hide resolved
class CliSpec < SpecWithProject
describe "Tapioca::Cli" do
before do
@command_stub = mock(run: nil)
Copy link
Member

Choose a reason for hiding this comment

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

This is just my preference, but I would avoid the extra dependency and just use Minitest::Mock here. I don't know what's the opinion for the rest of the team

@andyw8
Copy link
Contributor Author

andyw8 commented Nov 29, 2024

As suggested by @Morriar, I'm going to try using a single spec for all the LSP behavior.

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

Successfully merging this pull request may close these issues.

2 participants