Skip to content
This repository has been archived by the owner on Jul 15, 2023. It is now read-only.

Added support for tests using stretchr/testify #1707

Merged
merged 6 commits into from
Jun 20, 2018

Conversation

mhr3
Copy link
Contributor

@mhr3 mhr3 commented Jun 3, 2018

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.

image

@msftclas
Copy link

msftclas commented Jun 3, 2018

CLA assistant check
All CLA requirements met.

@mhr3
Copy link
Contributor Author

mhr3 commented Jun 8, 2018

@ramya-rao-a Could you review please?

@ramya-rao-a
Copy link
Contributor

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

@ramya-rao-a
Copy link
Contributor

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

@ramya-rao-a
Copy link
Contributor

Never mind, I used the one from your screenshot above

src/testUtils.ts Outdated
*/
export function extractInstanceTestName(symbol: vscode.SymbolInformation | string): string {
let symbolName = typeof symbol === 'string' ? symbol : symbol.name;
const match = symbolName.match(/\.(Test.*)$/);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/goTest.ts Outdated
@@ -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';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isInstanceTestMethod is an unused import here

src/testUtils.ts Outdated
*
* @param symbol Symbol to check.
*/
export function isInstanceTestMethod(symbol: vscode.SymbolInformation | string): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/testUtils.ts Outdated
*
* @param symbol Symbol to extract method name from.
*/
export function extractInstanceTestName(symbol: vscode.SymbolInformation | string): string {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/testUtils.ts Outdated
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@ramya-rao-a ramya-rao-a left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
Copy link
Contributor Author

mhr3 commented Jun 11, 2018

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
Copy link
Contributor

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
Copy link
Contributor Author

mhr3 commented Jun 11, 2018

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
Copy link
Contributor

ramya-rao-a commented Jun 11, 2018

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.

Copy link
Contributor

@ramya-rao-a ramya-rao-a left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

mhr3 commented Jun 12, 2018

Pushed the changes you requested, have another look please

src/testUtils.ts Outdated
const instanceType = match[1].replace(/[*]/g, '').toLowerCase();
const filtered = candidates.filter(t => {
const text = doc.getText(t.location.range).toLowerCase();
if (text.includes(instanceType)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/testUtils.ts Outdated
// filter further to ones containing suite.Run()
const candidates = testFunctions.filter(t => {
const text = doc.getText(t.location.range);
return /suite\.Run\(/.test(text);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

mhr3 commented Jun 18, 2018

@ramya-rao-a Anything still missing here?

@ramya-rao-a
Copy link
Contributor

ramya-rao-a commented Jun 18, 2018

@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
Copy link
Contributor Author

mhr3 commented Jun 19, 2018

Ok, refactored per your comment.

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

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

@alex-kovoy
Copy link

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

@ramya-rao-a
Copy link
Contributor

@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
Copy link

alex-kovoy commented Sep 4, 2018

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
Copy link
Contributor

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants