Skip to content

Commit 5a8c717

Browse files
vinistockvinistock
authored
Escape file paths for test command resolution (#3802)
Escape file paths for test command resolution (#3802) ### Motivation We weren't escaping file paths for test execution every time, which means paths including special characters (like parenthesis), failed to execute properly. ### Implementation We need to ensure we always escape paths or else we can't execute some less common, yet valid, file paths. ### Automated Tests Added tests. Co-authored-by: vinistock <18742907+vinistock@users.noreply.github.com>
1 parent 1445d5d commit 5a8c717

File tree

2 files changed

+175
-7
lines changed

2 files changed

+175
-7
lines changed

‎lib/ruby_lsp/listeners/test_style.rb‎

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -33,13 +33,15 @@ def resolve_test_commands(items)
3333

3434
if tags.include?("test_dir")
3535
if children.empty?
36-
full_files.concat(Dir.glob(
37-
"#{path}/**/{*_test,test_*,*_spec}.rb",
38-
File::Constants::FNM_EXTGLOB | File::Constants::FNM_PATHNAME,
39-
))
36+
full_files.concat(
37+
Dir.glob(
38+
"#{path}/**/{*_test,test_*,*_spec}.rb",
39+
File::Constants::FNM_EXTGLOB | File::Constants::FNM_PATHNAME,
40+
).map! { |p| Shellwords.escape(p) },
41+
)
4042
end
4143
elsif tags.include?("test_file")
42-
full_files << path if children.empty?
44+
full_files << Shellwords.escape(path) if children.empty?
4345
elsif tags.include?("test_group")
4446
# If all of the children of the current test group are other groups, then there's no need to add it to the
4547
# aggregated examples
@@ -114,7 +116,7 @@ def handle_minitest_groups(file_path, groups_and_examples)
114116
end
115117

116118
load_path = spec?(file_path) ? "-Ispec" : "-Itest"
117-
"#{COMMAND} #{load_path} #{file_path} --name \"/#{regex}/\""
119+
"#{COMMAND} #{load_path} #{Shellwords.escape(file_path)} --name \"/#{regex}/\""
118120
end
119121

120122
#: (String, Hash[String, Hash[Symbol, untyped]]) -> Array[String]
@@ -125,7 +127,7 @@ def handle_test_unit_groups(file_path, groups_and_examples)
125127
Shellwords.escape(TestDiscovery::DYNAMIC_REFERENCE_MARKER),
126128
".*",
127129
)
128-
command = +"#{COMMAND} -Itest #{file_path} --testcase \"/^#{group_regex}\\$/\""
130+
command = +"#{COMMAND} -Itest #{Shellwords.escape(file_path)} --testcase \"/^#{group_regex}\\$/\""
129131

130132
unless examples.empty?
131133
command << if examples.length == 1

‎test/requests/resolve_test_commands_test.rb‎

Lines changed: 166 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -753,6 +753,89 @@ def test_resolve_test_command_for_minitest_spec_with_backticks
753753
)
754754
end
755755
end
756+
757+
def test_resolve_properly_escapes_single_file_paths
758+
with_server do |server|
759+
server.process_message({
760+
id: 1,
761+
method: "rubyLsp/resolveTestCommands",
762+
params: {
763+
items: [
764+
{
765+
id: "file:///test/server(v2)_test.rb",
766+
uri: "file:///test/server(v2)_test.rb",
767+
label: "/test/server(v2)_test.rb",
768+
tags: ["test_file", "framework:minitest"],
769+
children: [],
770+
},
771+
],
772+
},
773+
})
774+
775+
result = server.pop_response.response
776+
assert_equal(
777+
["#{COMMAND} -Itest -e \"ARGV.each { |f| require f }\" /test/server\\(v2\\)_test.rb"],
778+
result[:commands],
779+
)
780+
end
781+
end
782+
783+
def test_resolve_properly_escapes_file_paths_within_directories
784+
with_server do |server|
785+
Dir.stubs(:glob).returns(["/other/test/fake(v2)_test.rb"])
786+
server.process_message({
787+
id: 1,
788+
method: "rubyLsp/resolveTestCommands",
789+
params: {
790+
items: [
791+
{
792+
id: "file:///other/test",
793+
uri: "file:///other/test",
794+
label: "/other/test",
795+
tags: ["test_dir", "framework:minitest"],
796+
children: [],
797+
},
798+
],
799+
},
800+
})
801+
802+
result = server.pop_response.response
803+
assert_equal(
804+
["#{COMMAND} -Itest -e \"ARGV.each { |f| require f }\" /other/test/fake\\(v2\\)_test.rb"],
805+
result[:commands],
806+
)
807+
end
808+
end
809+
810+
def test_resolve_properly_escapes_file_paths_in_groups
811+
with_server do |server|
812+
server.process_message({
813+
id: 1,
814+
method: "rubyLsp/resolveTestCommands",
815+
params: {
816+
items: [
817+
{
818+
id: "ServerTest",
819+
uri: "file:///test/server(v2)_test.rb",
820+
label: "ServerTest",
821+
range: {
822+
start: { line: 0, character: 0 },
823+
end: { line: 30, character: 3 },
824+
},
825+
tags: ["framework:minitest", "test_group"],
826+
children: [],
827+
},
828+
],
829+
},
830+
})
831+
832+
result = server.pop_response.response
833+
assert_equal(
834+
["#{COMMAND} -Itest /test/server\\(v2\\)_test.rb --name \"/^ServerTest(#|::)/\""],
835+
result[:commands],
836+
)
837+
end
838+
end
756839
end
757840

758841
class ResolveTestCommandsTestUnitTest < Minitest::Test
@@ -1254,6 +1337,89 @@ def test_resolve_test_command_mix_of_directories_and_examples
12541337
)
12551338
end
12561339
end
1340+
1341+
def test_resolve_properly_escapes_single_file_paths
1342+
with_server do |server|
1343+
server.process_message({
1344+
id: 1,
1345+
method: "rubyLsp/resolveTestCommands",
1346+
params: {
1347+
items: [
1348+
{
1349+
id: "file:///test/server(v2)_test.rb",
1350+
uri: "file:///test/server(v2)_test.rb",
1351+
label: "/test/server(v2)_test.rb",
1352+
tags: ["test_file", "framework:test_unit"],
1353+
children: [],
1354+
},
1355+
],
1356+
},
1357+
})
1358+
1359+
result = server.pop_response.response
1360+
assert_equal(
1361+
["#{COMMAND} -Itest -e \"ARGV.each { |f| require f }\" /test/server\\(v2\\)_test.rb"],
1362+
result[:commands],
1363+
)
1364+
end
1365+
end
1366+
1367+
def test_resolve_properly_escapes_file_paths_within_directories
1368+
with_server do |server|
1369+
Dir.stubs(:glob).returns(["/other/test/fake(v2)_test.rb"])
1370+
server.process_message({
1371+
id: 1,
1372+
method: "rubyLsp/resolveTestCommands",
1373+
params: {
1374+
items: [
1375+
{
1376+
id: "file:///other/test",
1377+
uri: "file:///other/test",
1378+
label: "/other/test",
1379+
tags: ["test_dir", "framework:test_unit"],
1380+
children: [],
1381+
},
1382+
],
1383+
},
1384+
})
1385+
1386+
result = server.pop_response.response
1387+
assert_equal(
1388+
["#{COMMAND} -Itest -e \"ARGV.each { |f| require f }\" /other/test/fake\\(v2\\)_test.rb"],
1389+
result[:commands],
1390+
)
1391+
end
1392+
end
1393+
1394+
def test_resolve_properly_escapes_file_paths_in_groups
1395+
with_server do |server|
1396+
server.process_message({
1397+
id: 1,
1398+
method: "rubyLsp/resolveTestCommands",
1399+
params: {
1400+
items: [
1401+
{
1402+
id: "ServerTest",
1403+
uri: "file:///test/server(v2)_test.rb",
1404+
label: "ServerTest",
1405+
range: {
1406+
start: { line: 0, character: 0 },
1407+
end: { line: 30, character: 3 },
1408+
},
1409+
tags: ["framework:test_unit", "test_group"],
1410+
children: [],
1411+
},
1412+
],
1413+
},
1414+
})
1415+
1416+
result = server.pop_response.response
1417+
assert_equal(
1418+
["#{COMMAND} -Itest /test/server\\(v2\\)_test.rb --testcase \"/^ServerTest\\$/\""],
1419+
result[:commands],
1420+
)
1421+
end
1422+
end
12571423
end
12581424

12591425
class ResolveTestCommandsOtherFrameworksTest < Minitest::Test

0 commit comments

Comments
 (0)