Added support for tests using stretchr/testify #1707

mhr3 · 2018-06-03T00:08:59Z

This adds support for tests using the widely used stretchr/testify framework, and allows running and debugging individual test methods with instances which use github.com/stretchr/testify/suite interfaces.

msftclas · 2018-06-03T00:09:11Z

All CLA requirements met.

mhr3 · 2018-06-08T11:38:49Z

@ramya-rao-a Could you review please?

ramya-rao-a · 2018-06-09T00:11:19Z

@mhr3 Yes, I'll get to this in a few days.

ramya-rao-a · 2018-06-11T02:23:58Z

@mhr3 Is there a sample repo where I can test this?

ramya-rao-a · 2018-06-11T04:44:30Z

Never mind, I used the one from your screenshot above

ramya-rao-a · 2018-06-11T04:49:02Z

src/testUtils.ts

+ */
+export function extractInstanceTestName(symbol: vscode.SymbolInformation | string): string {
+	let symbolName = typeof symbol === 'string' ? symbol : symbol.name;
+	const match = symbolName.match(/\.(Test.*)$/);


Why not re-use the testSuiteMethodRe regex here? We can do that if it is updated to /^$[^)]+$\.(Test.*)$/.

ramya-rao-a · 2018-06-11T04:49:23Z

src/goTest.ts

@@ -8,7 +8,7 @@
 import path = require('path');
 import vscode = require('vscode');
 import os = require('os');
-import { goTest, TestConfig, getTestFlags, getTestFunctions, getBenchmarkFunctions } from './testUtils';
+import { goTest, TestConfig, getTestFlags, getTestFunctions, getBenchmarkFunctions, isInstanceTestMethod } from './testUtils';


isInstanceTestMethod is an unused import here

ramya-rao-a · 2018-06-11T04:51:58Z

src/testUtils.ts

+ *
+ * @param symbol Symbol to check.
+ */
+export function isInstanceTestMethod(symbol: vscode.SymbolInformation | string): boolean {


Since this function isn't used anywhere, it need not be exported.

ramya-rao-a · 2018-06-11T05:04:43Z

src/testUtils.ts

+ *
+ * @param symbol Symbol to extract method name from.
+ */
+export function extractInstanceTestName(symbol: vscode.SymbolInformation | string): string {


All uses of this function passes in a string. So we don't need to support both string and vscode.SymbolInformation

ramya-rao-a · 2018-06-11T05:05:32Z

src/testUtils.ts

+			}
+
+			if (testFunctions.length > 0) {
+				params = params.concat(['-run', util.format('^%s$', testFunctions.join('|'))]);


We have to pass -run ^$ even if test.Functions array is empty. Otherwise all tests will be run

ramya-rao-a

@mhr3 Thanks for the PR, this will definitely be a great addition to this extension.

I have a left a few comments, and pushed a commit addressing the same. Please take a look.

If all looks good to you, then I'll merge this PR

mhr3 · 2018-06-11T09:35:48Z

Thanks for the review, and the refactor ;)

We have to pass -run ^$ even if test.Functions array is empty. Otherwise all tests will be run

That was intended, with -run ^$ none of the tests will run - including the ones that invoke suite.Run(...). Ideally we'd analyze the code deeper and match which Test function contains the necessary Run call, but that sounded like an overkill to me, you probably aren't mixing suite and non-suite tests in a single package - and even if, we'd just end up running more tests than necessary, which shouldn't be a big deal.

ramya-rao-a · 2018-06-11T15:46:30Z

you probably aren't mixing suite and non-suite tests in a single package - and even if, we'd just end up running more tests than necessary, which shouldn't be a big deal.

Take the case where the non testify test is failing and the user clicks on the "run test" codelens above a test which should be run using testify. Now the non-testify test will run and fail and the failure will be printed out in the output which can be misleading

mhr3 · 2018-06-11T16:59:44Z

Yes, I get what you're saying, but if we ran with -run ^$ -testify.m ^TestFoo$, nothing would actually run, cause testify relies on something invoking the suite.Run().

ramya-rao-a · 2018-06-11T22:18:45Z

but if we ran with -run ^$ -testify.m ^TestFoo$, nothing would actually run

Ah! I missed that. I understand it now.

Regardless, having all the normal tests run when the run test or debug test codelens are clicked or when the command Go: Test Function At Cursor is run, feels very wrong.

Especially because there is nothing enforcing the users to not use normal tests when using the suite from testify.

How about this?

When creating the codelens as well as in the testAtCursor function, we have access to all the test functions with their range.

If the current function is actually a method, then scan the text content of the other functions to find the one with suite.Run() call. Both names should get included in the TestConfig object in the testAtCursor function

This way, in targetArgs, you will have access to both the function and the method.

ramya-rao-a

Changing the Review state as we are still discussing the issue of all normal tests running when a single test from the suite is run

mhr3 · 2018-06-12T15:46:20Z

Pushed the changes you requested, have another look please

ramya-rao-a · 2018-06-12T16:50:45Z

src/testUtils.ts

+	const instanceType = match[1].replace(/[*]/g, '').toLowerCase();
+	const filtered = candidates.filter(t => {
+		const text = doc.getText(t.location.range).toLowerCase();
+		if (text.includes(instanceType)) {


This may give false positives if say there is a comment inside the function with another suite's name.
It can give false negatives if the suite was instantiated outside the function.

Why not return all the functions that have a suite.Run()?

Since we also pass -testify.m, the other suites will be a no-op right?

Changed, clearly this isn't going to work in 100% of cases, but should be close enough.

ramya-rao-a · 2018-06-12T16:52:26Z

src/testUtils.ts

+	// filter further to ones containing suite.Run()
+	const candidates = testFunctions.filter(t => {
+		const text = doc.getText(t.location.range);
+		return /suite\.Run\(/.test(text);


Since this is a simple string check, wont text.includes('suite.Run(') be simpler?
Unless we make this a useful regex like `/suite.Run([^\)]+)/

mhr3 · 2018-06-18T16:27:35Z

@ramya-rao-a Anything still missing here?

ramya-rao-a · 2018-06-18T18:06:08Z

@mh3 I dont see any changes related to my last comment #1707 (comment)

You have replied with "Changed, clearly this isn't going to work in 100% of cases, but should be close enough.", but I dont see which commit corresponds to this comment

Cases of false positives arent much of a problem. Since we pass testify.m only suites with the right method name will get run. I am concerned about false negatives where no suites will be run.

Therefore, I suggest to simplify the findTestFnForInstanceTest function.

Rename it to findAllTestSuiteRuns
It need not take symbolName as parameter
To filters given array of test functions and returns those that have a suite.Run() call in them

Then, we can revisit this later, if there is feedback on too many false positives

mhr3 · 2018-06-19T16:51:51Z

Ok, refactored per your comment.

ramya-rao-a · 2018-06-20T02:00:57Z

Thanks!
I'll release an update in a few days.

alex-kovoy · 2018-09-03T14:32:36Z

how can i disable now this new default behavior? You cannot run any tests now without this required framework

ramya-rao-a · 2018-09-04T06:14:47Z

@alex-kovoy That is not true. Tests not using the testify framework run just as before. If they don't for you, please log a new issue with a sample code we can try replicate your issue on.

alex-kovoy · 2018-09-04T13:14:20Z

It looks like this implementation is based on string parsing (regex) in order to determine if this framework is installed which I am not sure is the right way to implement this feature. Removing this line https://github.com/Microsoft/vscode-go/pull/1707/files#diff-869d156f8d53e34e379625eb2aab2d2cR293 fixed my problem.

ramya-rao-a · 2018-09-10T00:30:39Z

For anyone else seeing the same issue as @alex-kovoy, the issue is being tracked in #1911

Added support for testify

1d4f9eb

mhr3 force-pushed the testify-support branch from 6d45a37 to 1d4f9eb Compare June 3, 2018 12:12

ramya-rao-a suggested changes Jun 11, 2018

View reviewed changes

ramya-rao-a reviewed Jun 11, 2018

View reviewed changes

Refactoring

b109cab

ramya-rao-a approved these changes Jun 11, 2018

View reviewed changes

Add an explanation why we're skipping the -run param

29f7268

ramya-rao-a suggested changes Jun 11, 2018

View reviewed changes

Find matching test functions

273966b

mhr3 force-pushed the testify-support branch from 3d93904 to 273966b Compare June 12, 2018 15:45

ramya-rao-a reviewed Jun 12, 2018

View reviewed changes

Return all candidates

8d0721e

Refactor per review comments

714dd3a

ramya-rao-a approved these changes Jun 20, 2018

View reviewed changes

ramya-rao-a merged commit 47464f0 into microsoft:master Jun 20, 2018

alex-kovoy mentioned this pull request Sep 4, 2018

Cannot longer run unit-tests using check from vscode due to invalid stretchr/testify detection. #1911

Closed

ramya-rao-a mentioned this pull request Jun 10, 2020

code lens: support running and debugging tests that use http://labix.org/gocheck golang/vscode-go#111

Open

hyangah mentioned this pull request Jun 16, 2020

Add lenses for ginkgo to run specific tests golang/vscode-go#228

Open

breml mentioned this pull request Nov 9, 2020

testify test suite test cases are not recognized, if import is missing in file golang/vscode-go#899

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for tests using stretchr/testify #1707

Added support for tests using stretchr/testify #1707

mhr3 commented Jun 3, 2018

msftclas commented Jun 3, 2018 •

edited

Loading

mhr3 commented Jun 8, 2018

ramya-rao-a commented Jun 9, 2018

ramya-rao-a commented Jun 11, 2018

ramya-rao-a commented Jun 11, 2018

ramya-rao-a Jun 11, 2018

ramya-rao-a Jun 11, 2018

ramya-rao-a Jun 11, 2018

ramya-rao-a Jun 11, 2018

ramya-rao-a Jun 11, 2018

ramya-rao-a left a comment

mhr3 commented Jun 11, 2018 •

edited

Loading

ramya-rao-a commented Jun 11, 2018

mhr3 commented Jun 11, 2018

ramya-rao-a commented Jun 11, 2018 •

edited

Loading

ramya-rao-a left a comment

mhr3 commented Jun 12, 2018

ramya-rao-a Jun 12, 2018

mhr3 Jun 12, 2018

ramya-rao-a Jun 12, 2018

mhr3 commented Jun 18, 2018

ramya-rao-a commented Jun 18, 2018 •

edited

Loading

mhr3 commented Jun 19, 2018

ramya-rao-a commented Jun 20, 2018

alex-kovoy commented Sep 3, 2018

ramya-rao-a commented Sep 4, 2018

alex-kovoy commented Sep 4, 2018 •

edited

Loading

ramya-rao-a commented Sep 10, 2018

Added support for tests using stretchr/testify #1707

Added support for tests using stretchr/testify #1707

Conversation

mhr3 commented Jun 3, 2018

msftclas commented Jun 3, 2018 • edited Loading

mhr3 commented Jun 8, 2018

ramya-rao-a commented Jun 9, 2018

ramya-rao-a commented Jun 11, 2018

ramya-rao-a commented Jun 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramya-rao-a left a comment

Choose a reason for hiding this comment

mhr3 commented Jun 11, 2018 • edited Loading

ramya-rao-a commented Jun 11, 2018

mhr3 commented Jun 11, 2018

ramya-rao-a commented Jun 11, 2018 • edited Loading

ramya-rao-a left a comment

Choose a reason for hiding this comment

mhr3 commented Jun 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhr3 commented Jun 18, 2018

ramya-rao-a commented Jun 18, 2018 • edited Loading

mhr3 commented Jun 19, 2018

ramya-rao-a commented Jun 20, 2018

alex-kovoy commented Sep 3, 2018

ramya-rao-a commented Sep 4, 2018

alex-kovoy commented Sep 4, 2018 • edited Loading

ramya-rao-a commented Sep 10, 2018

msftclas commented Jun 3, 2018 •

edited

Loading

mhr3 commented Jun 11, 2018 •

edited

Loading

ramya-rao-a commented Jun 11, 2018 •

edited

Loading

ramya-rao-a commented Jun 18, 2018 •

edited

Loading

alex-kovoy commented Sep 4, 2018 •

edited

Loading