Wrap options_string values in quotes #633
Conversation
There was a problem hiding this comment.
Can we also add a test showing how Sorbet behaves when we run it? Maybe through tc test? https://github.com/Shopify/spoom/blob/cbe88be63783e0e593780b4151e6b98251de0f60/test/spoom/cli/srb/tc_test.rb
KaanOzkan
force-pushed
the
ko/quote-config
branch
from
b2fdd8b to
4c8b961
Compare
test/spoom/cli/srb/coverage_test.rb
Outdated
| @project.remove!("sorbet/config") | ||
| result = @project.spoom("srb coverage report --no-color") | ||
| assert_empty(result.out) | ||
| assert_equal("Error: not in a Sorbet project (`sorbet/config` not found)", result.err&.lines&.first&.chomp) | ||
| refute(result.status) | ||
|
|
||
| # Restore original config | ||
| @project.write!("sorbet/config", sorbet_config) |
There was a problem hiding this comment.
I don't know why removing the config didn't cause problems before. I can't find anywhere where we add . to the sorbet/config again.
There was a problem hiding this comment.
Not sure why we need this, every test should be reinitialized with a proper config using new_project https://github.com/Shopify/spoom/blob/main/test/test_helper.rb#L18-L24.
There was a problem hiding this comment.
I thought it was called just once at the beginning
spoom/test/test_with_project.rb
Line 20 in 7afab9d
There was a problem hiding this comment.
When you run the test, a new instance should be created for each test case so the initialize method should be called for each test_ method, you can put a print in new_project to confirm.
| @@ -488,6 +492,45 @@ def test_finish_on_original_branch | |||
| assert_equal("fake-branch", @project.git_current_branch) | |||
| end | |||
|
|
|||
| def test_config_options_string | |||
There was a problem hiding this comment.
@Morriar I went with a coverage test since it uses the options parsing. This test checks that the metrics are altered after ignoring one of the files.
test/spoom/cli/srb/coverage_test.rb
Outdated
| @project.remove!("sorbet/config") | ||
| result = @project.spoom("srb coverage report --no-color") | ||
| assert_empty(result.out) | ||
| assert_equal("Error: not in a Sorbet project (`sorbet/config` not found)", result.err&.lines&.first&.chomp) | ||
| refute(result.status) | ||
|
|
||
| # Restore original config | ||
| @project.write!("sorbet/config", sorbet_config) |
There was a problem hiding this comment.
Not sure why we need this, every test should be reinitialized with a proper config using new_project https://github.com/Shopify/spoom/blob/main/test/test_helper.rb#L18-L24.
test/spoom/cli/srb/coverage_test.rb
Outdated
| def test_config_options_string | ||
| sorbet_config = @project.read("sorbet/config") | ||
| # Add ignore path to config to test config options string | ||
| @project.write!("sorbet/config", "\n--ignore=lib/a.rb", append: true) |
There was a problem hiding this comment.
Should we try with a wildcard * to show the expected behavior?
There was a problem hiding this comment.
Done below as a new test.
There was a problem hiding this comment.
The coverage tests are kind of expensive, do you mind merging both of them please?
KaanOzkan
force-pushed
the
ko/quote-config
branch
2 times, most recently
from
aa9fc58 to
c4791d3
Compare
KaanOzkan
force-pushed
the
ko/quote-config
branch
from
c4791d3 to
e2518a0
Compare
No description provided.