fix regex flag parsing #74

[sockol]

Passed in flags like goblin.run are never parsed because the default testing package calls flag.Parse() before goblin calls flag.Parse(), which means the runRegex flag never gets set.

Resolves #74

Test manually: 1) Make a new test file like flag_test.go

package goblin

import (
    "testing"
)

func TestFlagRegex(t *testing.T) {
    g := Goblin(t)

    g.Describe("Test", func() {
        g.It("Auth 1", func() {})
        g.It("Auth 2", func() {})
        g.It("Mismatched test", func() {})
    })
}

2) run go test -timeout 30s -run ^TestFlagRegex -v -goblin.run=Auth 3) the output will have Auth 1 and Auth 2, but not Mismatched test

Some exploration of the bug:

// Add this into the end of the Goblin() function 
fmt.Println("......", os.Args[1:], timeout, *regexParam, "[", runRegex, "]", *regexParam != "")

run go test -run ^TestHandleItRegexExec -goblin.run="TestsrunPass"

...... [-test.timeout=10m0s -test.run=^TestHandleItRegexExec -goblin.run=TestsrunPass] 5s TestsrunPass [ <nil> ] true

  TestItRegexExec Helper
    ✓ TestsrunFail
    ✓ TestsrunPass
    ✓ TestsrunPass

 3 tests complete (0 ms)
--- FAIL: TestHandleItRegexExec (0.00s)
    goblin_test.go:480: Failed
FAIL
exit status 1
FAIL    _/Users/sockol/Desktop/Projects/goblin  0.293s

So this test doesnt parse the "runRegex" since flag.Parse() has been called

if !flag.Parsed() {
    parseFlags() // never runs, never sets "runRegex"
}

If we adjust the condition to be

func Goblin(t *testing.T, arguments ...string) *G {
    parseFlags()
    ...

func parseFlags() {
    //Flag parsing
    if !flag.Parsed() {
        flag.Parse()
    }
    if *regexParam != "" {
        runRegex = regexp.MustCompile(*regexParam)
    } else {
        runRegex = nil
    }
}

The command works as expected

 ~/De/P/goblin  on master *10 !2  go test -run ^TestHandleItRegexExec -goblin.run="TestsrunPass"
...... [-test.timeout=10m0s -test.run=^TestHandleItRegexExec -goblin.run=TestsrunPass] 5s TestsrunPass [ TestsrunPass ] true

  TestItRegexExec Helper
    ✓ TestsrunPass
    ✓ TestsrunPass

 2 tests complete (0 ms)
PASS
ok      _/Users/sockol/Desktop/Projects/goblin  0.135s

But the tests break - the new args passed into the "TestRegex" test are not parsed, [ runRegex] is not set

......  [-test.timeout=10m0s -goblin.run=matches] 5s  [ <nil> ] false

  Test
    1) Doesn't match regex
    ✓ It matches regex
    ✓ It also matches

 2 tests complete (0 ms)
 1 tests failed:

  1) Test Doesn't match regex:

    Regex shouldn't match
        /Users/sockol/Desktop/Projects/goblin/goblin_test.go:312 +0x42
        /Users/sockol/Desktop/Projects/goblin/goblin.go:231 +0x27
        /Users/sockol/Desktop/Projects/goblin/goblin.go:231 +0x3e7
--- FAIL: TestRegex (0.00s)
    goblin_test.go:320: Failed

[marcosnils]

LGTM. I'm wondering why travis didn't run for this check.

Does it make sense to add a test to make sure this doesn't break again?

[marcosnils]

Seems like the build failed but the check never reported the status here. https://travis-ci.org/github/franela/goblin/builds/669559996

[sockol]

Added a test that more closely simulates passing in tags. It should catch if this error shows up again, but I am not sure if this is the best way handle it.

[sockol]

I am not sure why the build failed though. Let's see how this new build will do

[marcosnils]

This tests is still failing: https://travis-ci.org/github/franela/goblin/builds/670824469#L442-L448

[sockol]

Pushed some changes - the new approach is to move runRegex into matchRegex() It could be improved by not compiling regex every time that function is called by checking if the -goblin.run parameter changed, but this should work for now.

[marcosnils]

the new approach is to move runRegex into matchRegex() It could be improved by not compiling regex every time that function is called by checking if the -goblin.run parameter changed, but this should work for now.

I don't quite understand why we need to do this. Compiling the regex every time doesn't make a lot of sense since the regex won't change in the execution of the tests. Can we refactor so we make the compilation happen once :pray:

[sockol]

For record keeping - there seem to be a couple ways of working around this:

1) Build that regex on runtime (as you pointed out, not a good workaround) 2) Check if the flag has been set, and compile regex if so 3) Rework the tests to test flags the same way I wrote the TestItRegexExec test 4) Find a different way to spoof flags in tests

I was able to get option 2 working and adjusted this commit https://github.com/franela/goblin/pull/79/commits/cef5fa09466336ea3edfd1213abe4c152a7b3253 to only compile when the regex param changes.

[marcosnils]

@sockol I believe I've fixed it here without too many hacky stuff. WDYT? https://github.com/franela/goblin/pull/83

[marcosnils]

Fixed by #83