src/goTest: fix bugs in subtest handling
This change addresses two issues
1. When the subtest name includes regexp metacharacters,
they need to be escaped before passing as `-run` flag value
that assumes regexp. We split around '/' and then escape meta
characters using escapeRegExp.
2. A bug in getOrCreateSubTest's child search prevented
looking up a node for the nested subtest. We should've compared
label with label. Added comments to clarify the difference
between label and name.
Fixes golang/vscode-go#3070
Fixes golang/vscode-go#2624
For golang/vscode-go#3022 (even though the issue is for the codelens)
Change-Id: I250d1188e91679aee5704e1bccfcd2344cf1ae90
Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/546260
Reviewed-by: Suzy Mueller <suzmue@golang.org>
Commit-Queue: Hyang-Ah Hana Kim <hyangah@gmail.com>
TryBot-Result: kokoro <noreply+kokoro@google.com>
diff --git a/src/goTest/resolve.ts b/src/goTest/resolve.ts
index 48c38bf..5bcc91c 100644
--- a/src/goTest/resolve.ts
+++ b/src/goTest/resolve.ts
@@ -169,27 +169,34 @@
return it(this.items);
}
- // Create or Retrieve a sub test or benchmark. The ID will be of the form:
+ // Create or Retrieve a sub test or benchmark. This is always dynamically
+ // called while processing test run output.
+ // The parent's uri is of the form:
// file:///path/to/mod/file.go?test#TestXxx%2fA%2fB%2fC
- getOrCreateSubTest(item: TestItem, label: string, name: string, dynamic?: boolean): TestItem | undefined {
- if (!item.uri) return;
- const { kind } = GoTest.parseId(item.id);
+ // The name is the name of the subtest to get or create.
+ // The label is the title presented in the UI.
+ // We expect the caller to call getOrCreateSubTest with the
+ // right parent.
+ // For example, if TestXxx has subtests and we are processing
+ // TestXxx/Sub1/Sub2, the parent item should be a node corresponding
+ // to TestXxx/Sub1, the label is Sub2 while the name is TestXxx/Sub1/Sub2.
+ getOrCreateSubTest(parent: TestItem, label: string, name: string): TestItem | undefined {
+ if (!parent.uri) return;
+ const { kind } = GoTest.parseId(parent.id);
let existing: TestItem | undefined;
- item.children.forEach((child) => {
- if (child.label === name) existing = child;
+ parent.children.forEach((child) => {
+ if (child.label === label) existing = child;
});
if (existing) return existing;
- item.canResolveChildren = true;
- const sub = this.createItem(label, item.uri, kind, name);
- item.children.add(sub);
+ parent.canResolveChildren = true;
+ const sub = this.createItem(label, parent.uri, kind, name);
+ parent.children.add(sub);
- if (dynamic) {
- this.isDynamicSubtest.add(sub);
- if (this.shouldSetRange(item)) {
- sub.range = item.range;
- }
+ this.isDynamicSubtest.add(sub);
+ if (this.shouldSetRange(parent)) {
+ sub.range = parent.range;
}
return sub;
diff --git a/src/goTest/run.ts b/src/goTest/run.ts
index 7f32f4b..697a481 100644
--- a/src/goTest/run.ts
+++ b/src/goTest/run.ts
@@ -28,6 +28,7 @@
import { debugTestAtCursor } from '../goTest';
import { GoExtensionContext } from '../context';
import path = require('path');
+import { escapeRegExp } from '../subTestUtils';
let debugSessionID = 0;
@@ -190,7 +191,7 @@
const run = this.ctrl.createTestRun(request, `Debug ${name}`);
if (!testFunctions) return;
- const started = await debugTestAtCursor(doc, name, testFunctions, goConfig, id);
+ const started = await debugTestAtCursor(doc, escapeSubTestName(name), testFunctions, goConfig, id);
if (!started) {
subs.forEach((s) => s.dispose());
run.end();
@@ -463,7 +464,7 @@
...rest,
outputChannel,
dir: pkg.uri?.fsPath ?? '',
- functions: Object.keys(functions),
+ functions: Object.keys(functions)?.map((v) => escapeSubTestName(v)),
goTestOutputConsumer: rest.isBenchmark
? (e) => this.consumeGoBenchmarkEvent(run, functions, complete, e)
: (e) => this.consumeGoTestEvent(run, functions, record, complete, concat, e)
@@ -503,12 +504,12 @@
const m = name.substring(pos).match(re);
if (!m) {
if (!parent) return tests[name];
- return this.resolver.getOrCreateSubTest(parent, name.substring(pos), name, true);
+ return this.resolver.getOrCreateSubTest(parent, name.substring(pos), name);
}
const subName = name.substring(0, pos + (m.index ?? 0));
const test = parent
- ? this.resolver.getOrCreateSubTest(parent, name.substring(pos, pos + (m.index ?? 0)), subName, true)
+ ? this.resolver.getOrCreateSubTest(parent, name.substring(pos, pos + (m.index ?? 0)), subName)
: tests[subName];
return resolve(test, pos + (m.index ?? 0), m[0].length);
};
@@ -722,3 +723,17 @@
return output.some((x) => rePkg.test(x));
}
}
+
+// escapeSubTestName escapes regexp-like metacharacters. Unlike
+// escapeSubTestName in subTestUtils.ts, this assumes the input are
+// coming from the test explorer test items whose names are computed from
+// the actual test run, not from a hacky source code analysis so escaping
+// empty unprintable characters is not necessary here.
+function escapeSubTestName(v: string) {
+ return v?.includes('/')
+ ? v
+ .split('/')
+ .map((part) => escapeRegExp(part), '')
+ .join('/')
+ : v;
+}
diff --git a/src/subTestUtils.ts b/src/subTestUtils.ts
index 76d8748..a81a6e4 100644
--- a/src/subTestUtils.ts
+++ b/src/subTestUtils.ts
@@ -18,6 +18,12 @@
return `${testFuncName}/${subTestName}`
.replace(/\s/g, '_')
.split('/')
- .map((part) => `\\Q${part}\\E`, '')
+ .map((part) => escapeRegExp(part), '')
.join('$/^');
}
+
+// escapeRegExp escapes regex metacharacters.
+// https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Regular_Expressions#escaping
+export function escapeRegExp(v: string) {
+ return v.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
+}
diff --git a/test/gopls/goTest.run.test.ts b/test/gopls/goTest.run.test.ts
index f1d2efe..112959a 100644
--- a/test/gopls/goTest.run.test.ts
+++ b/test/gopls/goTest.run.test.ts
@@ -230,15 +230,21 @@
// Locate subtest
console.log('Locate subtest');
- const tSub = tMain.children.get(GoTest.id(uri, 'test', 'TestMain/Sub'));
+ const tSub = tMain.children.get(GoTest.id(uri, 'test', 'TestMain/Sub|Test'));
assert(tSub, 'Subtest was not created');
console.log('Locate subtests with conflicting names');
- const tSub2 = tMain.children.get(GoTest.id(uri, 'test', 'TestMain/Sub#01'));
+ const tSub2 = tMain.children.get(GoTest.id(uri, 'test', 'TestMain/Sub|Test#01'));
assert(tSub2, 'Subtest #01 was not created');
- const tSub3 = tMain.children.get(GoTest.id(uri, 'test', 'TestMain/Sub#01#01'));
+ const tSub3 = tMain.children.get(GoTest.id(uri, 'test', 'TestMain/Sub|Test#01#01'));
assert(tSub3, 'Subtest #01#01 was not created');
+ const tSub4 = tMain.children.get(GoTest.id(uri, 'test', 'TestMain/1_+_1'));
+ assert(tSub4, 'Subtest 1_+_1 was not created');
+
+ const tSub5 = tSub4.children.get(GoTest.id(uri, 'test', 'TestMain/1_+_1/Nested'));
+ assert(tSub5, 'Subtest 1_+_1/Nested was not created');
+
// Run subtest by itself
console.log('Run subtest by itself');
assert(
@@ -255,7 +261,7 @@
console.log('Verify TestMain/Sub was run');
call = spy.lastCall.args[0];
assert.strictEqual(call.dir, subTestDir);
- assert.deepStrictEqual(call.functions, ['TestMain/Sub']);
+ assert.deepStrictEqual(call.functions, ['TestMain/Sub\\|Test']); // | is escaped.
spy.resetHistory();
// Ensure the subtest hasn't been disposed
diff --git a/test/testdata/subTest/sub_test.go b/test/testdata/subTest/sub_test.go
index ce66fe9..1aad473 100644
--- a/test/testdata/subTest/sub_test.go
+++ b/test/testdata/subTest/sub_test.go
@@ -4,9 +4,13 @@
func TestMain(t *testing.T) {
t.Log("Main")
- t.Run("Sub", func(t *testing.T) { t.Log("Sub") })
- t.Run("Sub", func(t *testing.T) { t.Log("Sub#01") })
- t.Run("Sub#01", func(t *testing.T) { t.Log("Sub#01#01") })
+ t.Run("Sub|Test", func(t *testing.T) { t.Log("Sub") })
+ t.Run("Sub|Test", func(t *testing.T) { t.Log("Sub#01") })
+ t.Run("Sub|Test#01", func(t *testing.T) { t.Log("Sub#01#01") })
+
+ t.Run("1 + 1", func(t *testing.T) {
+ t.Run("Nested", func(t *testing.T) { t.Log("1 + 1 = 2") })
+ })
}
func TestOther(t *testing.T) {
diff --git a/test/unit/subTestUtils.test.ts b/test/unit/subTestUtils.test.ts
index 7043cb4..4eb19cf 100644
--- a/test/unit/subTestUtils.test.ts
+++ b/test/unit/subTestUtils.test.ts
@@ -12,12 +12,17 @@
{
test: 'TestFunction',
subtest: 'GET /path/with/slashes',
- want: '\\QTestFunction\\E$/^\\QGET_\\E$/^\\Qpath\\E$/^\\Qwith\\E$/^\\Qslashes\\E'
+ want: 'TestFunction$/^GET_$/^path$/^with$/^slashes'
},
{
test: 'TestMain',
subtest: 'All{0,1} tests [run]+ (fine)',
- want: '\\QTestMain\\E$/^\\QAll{0,1}_tests_[run]+_(fine)\\E'
+ want: 'TestMain$/^All\\{0,1\\}_tests_\\[run\\]\\+_\\(fine\\)'
+ },
+ {
+ test: 'TestMain',
+ subtest: 'Foo|Bar+',
+ want: 'TestMain$/^Foo\\|Bar\\+'
}
];
