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 #3070
Fixes #2624
For #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 <[email protected]>
Commit-Queue: Hyang-Ah Hana Kim <[email protected]>
TryBot-Result: kokoro <[email protected]>

Author: hyangah

src/goTest/resolve.ts

@@ -169,27 +169,34 @@ export class GoTestResolver {
 		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;

src/goTest/run.ts

@@ -28,6 +28,7 @@ import { GoTestProfiler, ProfilingOptions } from './profile';
 import { debugTestAtCursor } from '../goTest';
 import { GoExtensionContext } from '../context';
 import path = require('path');
+import { escapeRegExp } from '../subTestUtils';
 
 let debugSessionID = 0;
 
@@ -190,7 +191,7 @@ export class GoTestRunner {
 
 		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 @@ export class GoTestRunner {
 			...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 @@ export class GoTestRunner {
 			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 @@ export class GoTestRunner {
 		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;
+}

src/subTestUtils.ts

@@ -18,6 +18,12 @@ export function escapeSubTestName(testFuncName: string, subTestName: string): st
 	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, '\\$&');
+}

test/gopls/goTest.run.test.ts

@@ -230,15 +230,21 @@ suite('Go Test Runner', () => {
 
 			// 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 @@ suite('Go Test Runner', () => {
 			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

test/testdata/subTest/sub_test.go

@@ -4,9 +4,13 @@ import "testing"
 
 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) {

test/unit/subTestUtils.test.ts

@@ -12,12 +12,17 @@ suite('escapeSubTestName Tests', () => {
 			{
 				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\\+'
 			}
 		];