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\\+' } ];