CheckError prevents negative testing

[mikeatlas]

I'd like to do some negative testing with forest, but since all the HTTP methods implicitly call CheckError(...), and function that directly calls t.Error(...), I can't get around this without writing my own versions of .GET (etc) without instead returning (response, err).

For example, I wish I could have the following:

// GET sends a Http request using a config (headers,...)
// The request is logged and any sending error will *be returned as not nil*.
func (a *APITesting) GET(t T, config *RequestConfig) (*http.Response, error) {
    httpReq, err := http.NewRequest("GET", a.BaseURL+config.pathAndQuery(), nil)
    if err != nil {
        t.Fatal(sfatalf("GET: invalid Url:%s", a.BaseURL+config.pathAndQuery()))
    }
    copyHeaders(config.HeaderMap, httpReq.Header)
    t.Logf("\n%v %v %v", httpReq.Method, httpReq.URL, headersString(httpReq.Header))
    resp, err := a.client.Do(httpReq)
    return resp, err // note the returned err
}

And then, I suppose, there needs to be either additional assertion method along the lines of:

func ExpectError(t T, r *http.Response, err error) bool

Thoughts?

[emicklei]

I understand your use case and your proposal would have made it easier to realize that. Separating the work (doing the call) and the checks is actually a better design. However, such a change will break existing usage of this package, so I need to consider any alternatives too.

One such alternative would be to wrap the t struct in one that has a different implemenation.

type ErrorCapture struct {
    Testing T
    ErrorArguments []interface{}
    LogFormat string
    LogArguments []interface{}
    FatalArguments []interface{}
}

func CaptureErrors(t T) T {
    return &ErrorCapture{Testing:T}
}

func (e *ErrorCapture) Error(args ...interface{}) {
    e.ErrorArguments = args 
}

func (e *ErrorCapture) Logf(format string, args ...interface{}) {
    e.LogFormat = string
    e.LogArguments = args
}

func (e *ErrorCapture) Fatal(args ...interface{}) {
    e.FatalArguments = args
}

Such that one can write something like:

cap := CaptureErrors(t)
resp := api.GET(cap,config)

What else can we think of?

[mikeatlas]

BTW, thanks for the library :) forgot to say that upfront.

I'm not sure really - maintaining backwards compatibility is easier when there's canonical package management and semantic versioning used - a major version release number bump would make it possible to break the function signature contract or behavior while providing an escape hatch from being tied forever to your first version... I've seen some Go libraries ship all their code under a "v1" namespaced/directory package, presumably so that anything in the v2 directory package version doesn't have to be backwards compatible, but at least you can move forward with new changes. Seems gross to me.

My first inclination (still relatively new to Go myself) was to suggest function overloads, but that's not supported in Go, others have suggested structs, but I'd personally rather just have more idiomatic (resp, err) return values and different methods to call, so I suppose the only real simple way to support negative tests going forward while maintaining backwards compatibility support would be to create mostly copy/paste alternative function with a different name that additionally return the err, but are compatible with the rest of your assertion checkers.

[mikeatlas]

Unfortunately that would violate DRY principles, but at the same time, Go principles favor readability and simplicity; and sometimes achieving DRY introduces complexity and reduces readability.

I can see why the language specification and designers opted not to do function overloading, but it's a shame in this situation - your original code isn't bad, but having been adopted as a public library, is sort of stuck the way it is.

[emicklei]

Yes, v1 + v2 is also an option.

How about adding another api such as

func (a *APITesting) Do(method string, config *RequestConfig) (*http.Response, error) {
    httpReq, err := http.NewRequest(method, a.BaseURL+config.pathAndQuery(), config.BodyReader)
    if err != nil {
        return nil, err
    }
    copyHeaders(config.HeaderMap, httpReq.Header)
    return a.client.Do(httpReq)
}

No logging, no error handling. More control.

[emicklei]

Another option is to tell the config to skip the check error call ; the GET of APITesting will check that

[mikeatlas]

@emicklei Thought of configuration-based behavior as well, but you'll have smelly, uglier code to test and maintain introducing changes like that. The wholesale Do idea (just flat out alternative function calls) you proposed above might indeed be the best way, and internally you could refactor the existing code to reuse the Do while maintaining existing behavior.

[mikeatlas]

Also, maybe consider adopting http://labix.org/gopkg.in ? It seems to address the issue of stable package versioning without the above messy solutions. You could do branch releases or tagged releases going forward.

[emicklei]

I am closing this issue as the provided Do(..) method allows negative testing.

[mikeatlas]

Thank you very much!