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

Add go.testOnSave option #810

Merged
merged 1 commit into from
Feb 26, 2017
Merged

Conversation

dmage
Copy link
Contributor

@dmage dmage commented Feb 22, 2017

This is a very straightforward implementation of the testOnSave feature (#581).

I suppose it would be much better to compile sources only one time if buildOnSave, testOnSave, and coverOnSave are on: go test -c includes everything from go build, cover is obtained from tests.

Also it would be nice to have some sort of progress bar, but I have no idea where it should be placed.

@msftclas
Copy link

Hi @dmage, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

@msftgits
Copy link
Contributor

Hi, I am closing and re-opening this PR to bump the CLA bot. Sorry for the inconvenience!

@msftgits msftgits closed this Feb 22, 2017
@msftgits msftgits reopened this Feb 22, 2017
@msftclas
Copy link

Hi @dmage, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!
We will now validate the agreement and then real humans will evaluate your PR.

TTYL, MSBOT;

@dmage dmage force-pushed the test-on-save branch 3 times, most recently from bb2743e to 886627c Compare February 24, 2017 19:16
@dmage
Copy link
Contributor Author

dmage commented Feb 24, 2017

As I can see there is no way to use same command and same handler for all three cases (the output of go test -c -cover differs from the output of go test -c/go build). I think it will be better to leave buildOnSave handler as is, later it can be replaced by go-langserver.

@dmage dmage force-pushed the test-on-save branch 4 times, most recently from 629ad39 to 68d8a6b Compare February 25, 2017 23:48
Signed-off-by: Oleg Bulatov <oleg@bulatov.me>
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.

This is so cool, thanks @dmage!

Just one thing, the messaging on the status item when tests are completed just say OK/FAIL. For someone just taking a look at it, its not clear what exactly is ok or not ok. :)

Instead, something like "Tests Passed/Failed" would be better. I can take care of that.

@ramya-rao-a ramya-rao-a merged commit e40b400 into microsoft:master Feb 26, 2017
@dmage
Copy link
Contributor Author

dmage commented Feb 26, 2017

Thank you, I didn't expect you to merge this without any changes :)

Please take a look at dmage@d4a9ff3

Now I think showHideTestStatus is unnecessary. There is no reason to hide test status when a tab switched. And test status should be invalidated if any file was changed (it may be a fixture, for example).

@ramya-rao-a
Copy link
Contributor

To be honest I wanted to include this feature in the next update which I wanted to release in a day or two. Didn't know if that was enough time for you to respond to any additional refactoring comments. So I thought I'd do it myself :)

Thanks for the update, that is definitely more cleaner. Feel free to send a PR, and I'll get it merged right away :)

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