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

code depending on "testing" package must be in _test.go files #268

Merged
merged 4 commits into from Feb 23, 2016

Conversation

gmarik
Copy link
Contributor

@gmarik gmarik commented Feb 22, 2016

Otherwise any code importing gobot package will have
testing flags as well:

Problem

Unexpected flags in my app:

     -test.bench string
        regular expression to select benchmarks to run
     -test.benchmem
        print memory allocations for benchmarks

Reproduction

Here's a minimum code to reproduce the problem:

package main
// main.go

import (
    "flag"
    "github.com/hybridgroup/gobot"
)

var _ = gobot.NewJSONDevice

func main() {
    flag.Parse()
    flag.PrintDefaults()
}

Output

flag_test$ go run main.go
  -test.bench string
        regular expression to select benchmarks to run
  -test.benchmem
        print memory allocations for benchmarks
  -test.benchtime duration
        approximate run time for each benchmark (default 1s)

# .....

Fix

Extract test helpers to gobot/gobottest package.

@deadprogram
Copy link
Member

Hi, @gmarik I like the concept, but as you can tell https://travis-ci.org/hybridgroup/gobot/jobs/110869133 that are many tests that depend on the current setup.

If you want to run all the tests on your machine, use the command make test

Perhaps a gobot/gobottest subdirectory would be a nice way to implement this refactoring.

Many tests would need to be rewritten from this change, however, sounds like that is the case no matter what, at this point.

What do you think?

@gmarik
Copy link
Contributor Author

gmarik commented Feb 22, 2016

@deadprogram yeah, i've noticed. Took me a bit to get tests failing.

Hacked together a quick-fix, but if you think we should do it the right way with gobottest package i'll rewrite it. Let me know.

@deadprogram
Copy link
Member

A separate test package would be so much nicer, if you please, and thank you!

@gmarik
Copy link
Contributor Author

gmarik commented Feb 22, 2016

@deadprogram done! (unless test suite fails)

Also please double check your latest merge is successfully fixed (as it introduced conflict on my side)

@gmarik
Copy link
Contributor Author

gmarik commented Feb 22, 2016

Any ideas why Is this failing? (looks like Publish order is not predictable)

--- FAIL: TestPublish (0.00s)
    gobottest.go:12: utils_test.go:51: 4 - "int", should equal,  1 - "int"

And this:

# git.eclipse.org/gitroot/paho/org.eclipse.paho.mqtt.golang.git
../../../git.eclipse.org/gitroot/paho/org.eclipse.paho.mqtt.golang.git/net.go:59: undefined: tls.DialWithDialer

@deadprogram
Copy link
Member

Not sure about the first fail, but the second is due to the mqtt package requires versions of Go >= 1.3, which is why 1.2 is in the Travis allows failure section: https://github.com/hybridgroup/gobot/blob/dev/.travis.yml#L13

@gmarik gmarik mentioned this pull request Feb 23, 2016
@gmarik gmarik closed this Feb 23, 2016
@gmarik gmarik reopened this Feb 23, 2016
@gmarik
Copy link
Contributor Author

gmarik commented Feb 23, 2016

@deadprogram please review!
Merged fix to this PR from #269

@deadprogram
Copy link
Member

This looks like the right idea to me. Any feedback @trevrosen or @mattetti or anyone else?

@trevrosen
Copy link
Collaborator

👍

Really happy to see some of this test stuff getting love. This will help me with some long-time goals that I'm woefully behind on. 😺

:shipit:

@deadprogram
Copy link
Member

🔍

deadprogram added a commit that referenced this pull request Feb 23, 2016
code depending on "testing" package must be in _test.go files
@deadprogram deadprogram merged commit 6c287a3 into hybridgroup:dev Feb 23, 2016
@gmarik
Copy link
Contributor Author

gmarik commented Feb 23, 2016

Thanks guys!

@deadprogram
Copy link
Member

Thanks to you, @gmarik

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.

None yet

3 participants