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

Ability to specify comment when asserting with So() #355

Closed
XANi opened this issue Oct 15, 2015 · 8 comments · Fixed by #451
Closed

Ability to specify comment when asserting with So() #355

XANi opened this issue Oct 15, 2015 · 8 comments · Fixed by #451

Comments

@XANi
Copy link

XANi commented Oct 15, 2015

When checking if multiple fields in output struct are correct it would be useful to have ability to add comment to So() instead of just getting a line number of failed assert eg.

tested, err := NewHostFromMap(m)
    Convey("HostFromMap", t, func() {
        So(err, ShouldEqual, nil)
        So(tested.Hostname, ShouldEqual, m["host_name"])
        So(tested.State, ShouldEqual, "WARNING")
        So(tested.PreviousState, ShouldEqual, "OK")
        So(tested.CheckMessage, ShouldEqual, "DUMMY CHECK WARNING")
        ...

will just generate failures with only line number as an info, to get meaningful feedback I have to use much more verbose (and IMO much less readable) format:

tested, err := NewServiceFromMap(m)
Convey("ServiceFromMap", t, func() {
    Convey("no err", func() {
        So(err, ShouldEqual, nil)
    })
    Convey("Hostname", func() {
        So(tested.Hostname, ShouldEqual, m["host_name"])
    })
    Convey("Description", func() {
        So(tested.Description, ShouldEqual, m["service_description"])
    })
    Convey("State", func() {
        So(tested.State, ShouldEqual, "WARNING")
    })
    ...

It would be nice if it would be possible just to have something like

        So(tested.Hostname, ShouldEqual, m["host_name"], "Hostname")
        So(tested.State, ShouldEqual, "WARNING", "State")

and have that last field as a comment next to filename:line

@mdwhatcott
Copy link
Collaborator

You know about ShouldResemble for comparing structs, right?

@XANi
Copy link
Author

XANi commented Oct 15, 2015

Well, I've tried:

shot0001

but I wouldn't call that an improvement as it only works if created struct is 100% identical to compared data.

Also it is kinda hard to even find a difference in that view

@mdwhatcott
Copy link
Collaborator

Ok, thanks for clarifying. I've struggled with this same frustrating problem.

Modifying the signature to the So call is a pretty significant change that would ripple all the way into every function of the assertions package (if you want the message to appear along with the line:file info. That's a lot to work through. So, while I'm not completely against this, I'm not sold on it yet either.

@XANi
Copy link
Author

XANi commented Oct 15, 2015

Maybe instead variation on Convey() (with name like ConveySimple, or ConveyOne or sth) that would work like Convey with single So:

Convey("Hostname", t, tested.Hostname, ShouldEqual, "testhost")

That would cut on convey blocks that are just single assertion without cutting down on readability

@mdwhatcott
Copy link
Collaborator

Yeah, we've actually thought about that too at one point. It was complicated when we discussed it at that time and the internals of the convey package have been almost completely rewritten since then so I don't think I could estimate the complexity of the task.

Perhaps you would consider creating a simple assertion function using the assertions package by itself (without the convey package)?

func assertEqual(expected, actual interface{}, message string, t *testing.T) {
    if ok, message := assertions.So(x.something, ShouldEqual, y.something); !ok {
        t.Error(message)
    }
}

@XANi
Copy link
Author

XANi commented Oct 16, 2015

Well, just adding

func ConveyOne(s string, c1 interface{}, assert assertion, c2 interface{}) {
    ctx := getCurrentContext()
    ctx.Convey(s, func() {
        ctx.So(c1, assert, c2)
    })
}

(and ofc adding to to C interface{} ) seems to work well enough altho it would require a bit more work to work in root context and I dont really know Go that well.

Also why there is code in doc.go ? Shouldn't that be "documentation only" file ? Took me ages to find where is the interface because every time that file popped up I assumed it is just some piece of docs and not actual function/interface

@mdwhatcott
Copy link
Collaborator

Good question about the doc.go file. The original thinking with that file was to put all the exported names there. The project has since grown. Maybe that file should be called convey.go now...

You are welcome to move forward with what you are proposing, taking care to not cause any breaking changes to the exported API/contract.

@kormat
Copy link
Contributor

kormat commented Oct 13, 2016

The ConveyOne-type solution doesn't solve the case where you're iterating over values. The only way to fix that that i can see is to go to the pain of adding a So variant that supports a message (maybe called SoMsg?). Otherwise you're stuck with the suggestion @mdwhatcott made here #355 (comment), and lose a lot of the convey benefits.

@kormat kormat mentioned this issue Oct 13, 2016
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 a pull request may close this issue.

3 participants