Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VCS-74: Add +build tags to tests, to enable selective per-VCS testing #75

Closed
wants to merge 1 commit into from

Conversation

m0j0hn
Copy link
Contributor

@m0j0hn m0j0hn commented Apr 17, 2017

Add Golang build constraints (build tags) to *_test.go, so one can run only the tests for the module being developed, instead of previous default of all.
Note that Makefile and appveyor.yml have been updated to run all tests, as before.
Note that running "git test" now does nothing by default - this is a change.

Syntax: Run all tests:
go test -tags all
Syntax: Run git tests:
go test -tags git

@AppVeyorBot
Copy link

git.go Outdated
s.log(out)
if err != nil {
return NewLocalError("Error while updating submodule sources", err, string(out))
}

Choose a reason for hiding this comment

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

Did you mean to include this change as part of this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope. Good catch!
Just pushed new revision without that -
please take another look when you get a chance -
and Thanks!

@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

@m0j0hn
Copy link
Contributor Author

m0j0hn commented Apr 19, 2017

@shurcooL I think this PR is in final form - can you please take another look at it and let me know +1 or -1 at this time - or if you and team are even open to the idea of "go test" doing nothing by default?
I know I like not having to install svn & bzr & hg just to work on the Git parts, but maybe I am being lazy ;)
Thanks!

@dmitshur
Copy link

I'm not an owner of this project, I just spotted an issue and wanted to point it out. You'll want to get a review from someone qualified to do it.

@m0j0hn
Copy link
Contributor Author

m0j0hn commented Apr 19, 2017

Ok, Thank you, @shurcooL!

@mattfarina @technosophos Can you please consider this PR? Thanks!

@m0j0hn m0j0hn changed the title VCS-74: Add +build tags to tests, to enable selective per-VCS testing VCS-75: Add +build tags to tests, to enable selective per-VCS testing Apr 20, 2017
@m0j0hn m0j0hn changed the title VCS-75: Add +build tags to tests, to enable selective per-VCS testing VCS-74: Add +build tags to tests, to enable selective per-VCS testing Apr 21, 2017
@AppVeyorBot
Copy link

@technosophos
Copy link
Member

I'm not opposed to this on principle. I've done something similar on database libraries where there were certain drivers (MySQL, sqlite, etc) that had specific tests.

But I do feel a little uncomfortable with the idea that a plain go test will apparently run no tests. This can give developers the false impression that all is well when it's not (though I personally always run make test). I would rather see the default be all with the option to turn off specific drivers. I guess I should get @mattfarina 's opinion on that, though. He's the architect of this library, and he may have different feelings about it.

@mattfarina
Copy link
Member

I agree with @technosophos on this one.

Imagine the case of a windows developer is working on this package. There is no make test. And, not all *nix developers will use that. Some will go with go test. There is extra cognitive load to figure out what's going on. You have to think by default.

Can we get this where you can turn off certain tests?

@mattfarina
Copy link
Member

Since I've not heard back on this PR since April (5 month) I'm going to close it. If there is new movement it can be reopened.

@mattfarina mattfarina closed this Sep 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants