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

Convey assertions fail inside an httptest server handler. #134

Closed
augustoroman opened this issue Feb 3, 2014 · 9 comments
Closed

Convey assertions fail inside an httptest server handler. #134

augustoroman opened this issue Feb 3, 2014 · 9 comments

Comments

@augustoroman
Copy link

Tests that used to work (at 64e6d43) using an httptest server fail on master with:

2014/02/02 21:30:58 http: panic serving 127.0.0.1:63508: Can't resolve test method name! Are you calling Convey() from a *_test.go file and a Test* method (because you should be)?

I'll work on creating a minimal example soon.

@mdwhatcott
Copy link
Collaborator

OK, good to know. Solving #70 required a few significant internal changes but it shouldn't be hard to resolve this. A example that displays the behavior would be very helpful.

@mdwhatcott
Copy link
Collaborator

Initial experimentation didn't yield anything interesting. This code works:

package examples

import (
    "fmt"
    "io/ioutil"
    "log"
    "net/http"
    "net/http/httptest"
    "strings"
    "testing"

    . "github.com/smartystreets/goconvey/convey"
)

func TestStuff(t *testing.T) {
    ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
        fmt.Fprintln(w, "Hello, client")
    }))
    defer ts.Close()

    Convey("HTTP Stuff", t, func() {
        res, err := http.Get(ts.URL)
        if err != nil {
            log.Fatal(err)
        }
        greeting, err := ioutil.ReadAll(res.Body)
        res.Body.Close()
        if err != nil {
            log.Fatal(err)
        }

        So(strings.TrimSpace(string(greeting)), ShouldEqual, "Hello, client")
    })
}

@mdwhatcott
Copy link
Collaborator

@augustoroman - Any updates for this issue?

@augustoroman
Copy link
Author

Got it. The http server was a bit of a red herring, the actual problem is assertions within the handler function:

package test

import (
    "net/http"
    "net/http/httptest"
    "testing"
    . "github.com/smartystreets/goconvey/convey"
)

func TestHttpServer(t *testing.T) {
    Convey("assertions should work in an embedded http server", t, func() {
        ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
            So(r.FormValue("msg"), ShouldEqual, "ping")
        }))
        http.DefaultClient.Get(ts.URL + "?msg=ping")
    })
}

fails with

2014/02/10 21:04:28 http: panic serving 127.0.0.1:55353: Can't resolve test method name! Are you calling Convey() from a `*_test.go` file and a `Test*` method (because you should be)?

@mdwhatcott
Copy link
Collaborator

@augustoroman - Ah, got it. The mechanism that links assertions to their test suites/scopes is a bit fragile. Right now it's accomplished by tracing the call stack and obviously, it should be made more robust. Thanks for the great example. I'll see what I can do.

@mdwhatcott
Copy link
Collaborator

Coincidentally, the changes that caused this issue were implemented to fix #70 (also you're issue @augustoroman :). I'm happy with the fix for #70 as it brings GoConvey into compatibility with go test but it also required some call stack craziness. I've thrown together a messy fix on the issue_134 branch. Let me know if that solves the problem (as the test I added says it should). If so, I'll clean it up and merge it into master.

mdwhatcott added a commit that referenced this issue Feb 13, 2014
Runner can call the Reporter with assertion results; suiteContext no
longer needs reporter collection; general cleanup;
This was referenced Nov 3, 2014
@leeola
Copy link

leeola commented Nov 14, 2014

@mdwhatcott Did #264 break this usage? (testing inside an httptest handler)

I'm not sure if this usage is still supported, so forgive me if it's not, but after updating goconvey my tests suddenly started throwing panics due to a patch (i believe #264) from ~11 days ago.

Can anyone confirm?

@mdwhatcott
Copy link
Collaborator

@leeolayvar - Yes and no--you can still get what this issue solved, but the API did change slightly

Here's the example from above, reworked with the new syntax.

func TestHttpServer(t *testing.T) {
    Convey("assertions should work in an embedded http server", t, func(c C) {
        ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
            c.So(r.FormValue("msg"), ShouldEqual, "ping")
        }))
        http.DefaultClient.Get(ts.URL + "?msg=ping")
    })
}

Notice that the func() is now func(c C)--it receives a context interface that helps tie things together when you are performing assertions across goroutines. This new signature is only necessary when multiple goroutines are in play. For every other case there is no change in usage.

I'm really sorry for the inconvenience. I felt it was for the greater good and progress of the convey package.

@leeola
Copy link

leeola commented Nov 14, 2014

No sorry needed, i prefer the more explicit nature of it! Especially to overcome fragile implementations.

Thanks for the explanation, it works quite well 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants