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

Enable testflags via settings and keyboard bindings#482

Merged
ramya-rao-a merged 1 commit intomicrosoft:masterfrom
ramya-rao-a:testflags
Jan 28, 2017
Merged

Enable testflags via settings and keyboard bindings#482
ramya-rao-a merged 1 commit intomicrosoft:masterfrom
ramya-rao-a:testflags

Conversation

@ramya-rao-a
Copy link
Copy Markdown
Contributor

@ramya-rao-a ramya-rao-a commented Sep 15, 2016

(Updated on 21/1/2017)

Fixes #401 and #683

Added a new setting go.testFlags
- If null, then all the test commands will continue to use the go.buildFlags and '-v' while running go test as always.
- Else, the provided flags will be used by the test commands instead of go.buildFlags while running go test

The usage of go.testTimeout and go.buildTags will continue to remain as is.

Users can also customize the flags and have keyboard shortcuts for the same.

{
        "key": "ctrl+shift+t",
        "command": "go.test.file",
        "args": {
            "flags": ["-short"]
        },
        "when": "editorTextFocus"
}

@ramya-rao-a ramya-rao-a changed the title Fixes #401 Add testFlags to user settings with -v as default Add testFlags to user settings with -v as default Sep 15, 2016
Copy link
Copy Markdown
Contributor

@ironcladlou ironcladlou left a comment

Choose a reason for hiding this comment

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

Let's talk about buildFlags in the context of this change...

Comment thread src/goTest.ts Outdated
let buildTags: string = vscode.workspace.getConfiguration('go')['buildTags'];
let args = ['test', '-v', '-timeout', config.timeout, '-tags', buildTags, ...buildFlags];
let testFlags: string[] = vscode.workspace.getConfiguration('go')['testFlags'];
let args = ['test', ...testFlags, '-timeout', config.timeout, '-tags', buildTags, ...buildFlags];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I like the idea of using testFlags for build/test flags to the test command. Should we remove support for buildFlags here, given there is no distinction in go test between build and test flags? Having both is redundant and confusing to me given your new option.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed. Ideally

  • buildFlags should be used for build only,
  • testFlags should be used for test only.
  • testTimeout should not be a separate setting and should be included in the testFlags setting

If we do this, we won't be backward compatible and everyone who had configured buildFlags and/or testTimeout will have to update their settings to add the new testFlags

@lukehoban Thoughts?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@ramya-rao-a

testTimeout should not be a separate setting and should be included in the testFlags setting

Hm. Wouldn't it then follow that buildTags should be merged into buildFlags? After all, -tag is just another buildFlag. I do see value in adding convenient strongly typed wrappers around the most common flags, but those would need to be easily omitted so that users can have absolute control over flags.

@ramya-rao-a ramya-rao-a force-pushed the master branch 2 times, most recently from 0b2155b to 7aa5605 Compare November 21, 2016 04:01
@ramya-rao-a ramya-rao-a changed the title Add testFlags to user settings with -v as default Enable testflags via settings and keyboard bindings Jan 22, 2017
@ramya-rao-a
Copy link
Copy Markdown
Contributor Author

cc @roblourens
ping @ironcladlou

@roblourens
Copy link
Copy Markdown
Member

LGTM

@ramya-rao-a
Copy link
Copy Markdown
Contributor Author

ramya-rao-a commented Jan 27, 2017

@ironcladlou I wish you had a chance to look at this PR. I am planning to release an update in a few days, therefore will go ahead and merge this PR so that I can do some end to end testing.

When you get some time, please do add your thoughts here.

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