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

feat: added testFlags option #1877

Merged
merged 4 commits into from
Sep 4, 2018
Merged

feat: added testFlags option #1877

merged 4 commits into from
Sep 4, 2018

Conversation

johan-lejdung
Copy link
Contributor

I tested it out locally and it worked fine and all tests except rungodoc passed but it looked like I had to do some setup for that to pass to that is most likely the reason.

@msftclas
Copy link

msftclas commented Aug 26, 2018 •
edited
Loading

CLA assistant check
All CLA requirements met.

@johan-lejdung
Copy link
Contributor Author

johan-lejdung commented Aug 26, 2018 •
edited
Loading

Resolves: #1842

ramya-rao-a
ramya-rao-a suggested changes Aug 30, 2018
View reviewed changes
src/testUtils.ts Outdated
@@ -166,7 +166,7 @@ export function goTest(testconfig: TestConfig): Thenable<boolean> {
outputChannel.show(true);
}

let buildTags: string = testconfig.goConfig['buildTags'];
let buildTags: string = testconfig.goConfig['testTags'];
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we should fallback to using buildTags if no testTags are provided to ensure we are not breaking anybody else's use case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course!

package.json Outdated
@@ -515,9 +515,15 @@
"go.buildTags": {
"type": "string",
"default": "",
"description": "The Go build tags to use for all commands that support a `-tags '...'` argument",
"description": "The Go build tags to use for all commands, excluding `go test` (see testTags), that support a `-tags '...'` argument",
Copy link
Contributor

Choose a reason for hiding this comment

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

buildtags will still be used for compiling the test files which will be using go test
So we should probably change this to excluding when tests are run or something to that effect

ramya-rao-a
ramya-rao-a approved these changes Aug 30, 2018
View reviewed changes
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.

Thanks!

ramya-rao-a
ramya-rao-a reviewed Aug 30, 2018
View reviewed changes
src/testUtils.ts Outdated
@@ -166,7 +166,7 @@ export function goTest(testconfig: TestConfig): Thenable<boolean> {
outputChannel.show(true);
}

let buildTags: string = testconfig.goConfig['buildTags'];
let testTags: string = (testconfig.goConfig.has("testTags")) ? testconfig.goConfig['testTags'] : testconfig.goConfig['buildTags'];
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to use single quotes, else the linter breaks the build.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops!

ramya-rao-a
ramya-rao-a reviewed Aug 30, 2018
View reviewed changes
package.json Outdated
@@ -515,9 +515,15 @@
"go.buildTags": {
"type": "string",
"default": "",
"description": "The Go build tags to use for all commands that support a `-tags '...'` argument",
"description": "The Go build tags to use for all commands, excluding when running tests (see testTags), that support a `-tags '...'` argument",
Copy link
Contributor

Choose a reason for hiding this comment

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

The description now is a little confusing. Can we move the exclude statement to the end? How about the below?

The Go build tags to use for all commands that support a -tags '...'argument. When running tests,go.testTags will be used instead if it was set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That does sound better :)

@johan-lejdung
Copy link
Contributor Author

The test with the multiple flags seems to be failing as well, not sure why since it's implemented just like buildTags. I will look at it later this weekend.

@ramya-rao-a
Copy link
Contributor

The testTags are to be used when running the tests. The unit tests are using the check function that is used to only compile the files and not run tests. Therefore, the check method will only use the build tags and will never use the test tags.

That's the reason for the failures

@johan-lejdung
Copy link
Contributor Author

Ah, thanks! Will get it sorted out today. Haven't had a second look yet, sorry for the delay :)

@ramya-rao-a
Copy link
Contributor

No problem, take your time.

@johan-lejdung
Copy link
Contributor Author

I removed the test for fallback on buildTags. The reason was that this line would evaluate to true even though 'testTags' was equal to ''.

First I chose to modify the check to

let testTags: string = testconfig.goConfig['testTags'].length > 1 ? testconfig.goConfig['testTags'] : testconfig.goConfig['buildTags'];

But that would create a weird dependency on buildTags, and remove the useCase we are looking to create; where you wanted tests to run without any tags but let it build the files with some tags.

Not sure how to solve the backwards compatibility problem actually since an unset config evaluated to an empty string and not undefined.

@johan-lejdung
Copy link
Contributor Author

johan-lejdung commented Sep 2, 2018 •
edited
Loading

Ehm, getting weird errors Expected 3 arguments, but got 2.
on

return testCurrentFile(config1, []).then((result: boolean) => {

The function definition is as follows:

export function testCurrentFile(goConfig: vscode.WorkspaceConfiguration, args: string[]): Thenable<boolean>

Any ideas?

@ramya-rao-a
Copy link
Contributor

Ehm, getting weird errors Expected 3 arguments, but got 2.

Merge from the master branch to fix that.

@ramya-rao-a
Copy link
Contributor

and remove the useCase we are looking to create; where you wanted tests to run without any tags but let it build the files with some tags.

We had a similar case with the go.testFlags setting. We ended up supported null as a valid value. See below:

screen shot 2018-09-02 at 2 50 04 am

@johan-lejdung
Copy link
Contributor Author

That should do it!

@johan-lejdung
Copy link
Contributor Author

When can I expect this to get released? :)

@ramya-rao-a ramya-rao-a merged commit eae427d into microsoft:master Sep 4, 2018
@ramya-rao-a
Copy link
Contributor

@johan-lejdung A release should be some time next week. Until then, you can use the beta version of the extension to use this feature. See https://github.com/Microsoft/vscode-go/wiki/Use-the-beta-version-of-the-latest-Go-extension

Thanks for working on this!

@johan-lejdung
Copy link
Contributor Author

johan-lejdung commented Sep 4, 2018 via email

@Rhionin
Copy link

Rhionin commented Sep 19, 2018

Bless you for building this! I've been dying for this feature!

@johan-lejdung
Copy link
Contributor Author

That's really nice to hear ☺️

@ramya-rao-a ramya-rao-a mentioned this pull request Sep 24, 2018
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ramya-rao-a ramya-rao-a ramya-rao-a approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@johan-lejdung @msftclas @ramya-rao-a @Rhionin