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

Use deep render method for Should[Not]Resemble. #6

Closed
wants to merge 1 commit into from

Conversation

danjacques
Copy link

This branch implements DeepRender, a method that uses reflection to safely render a type's contents recursively to string. It routes Should[Not]Resemble calls through DeepRender to provide more information in failure cases.

The Should[Not]Resemble assertions perform a reflect.DeepEquals, but the resulting output in the event of a mismatch doesn't provide any insight into the differences if they lie below the top-level struct.

For example,

func TestThings(t *testing.T) {
  type T struct {
    *string
    s *string
  }
  s0 := "foo"
  s1 := "bar"

  Convey(`test`, t, func() {
    So(T{&s0, &s1}, ShouldResemble, T{&s1, &s0})
  })
}

Yields:

Line 17:
  Expected: 'asdf.T{string:(*string)(0xc2080304e0), s:(*string)(0xc2080304e0)}'
  Actual:   'asdf.T{string:(*string)(0xc2080304d0), s:(*string)(0xc2080304e0)}'
  (Should resemble)!

I wrote most of this for a project of mine, accessed through custom assertions built on top of ShouldResemble and ShouldNotResemble, and have found it very useful so far. In this simple case, we get:

Line 17:
  Expected: 'asdf.T{string:(*string)("bar"), s:(*string)("foo")}'
  Actual:   'asdf.T{string:(*string)("foo"), s:(*string)("bar")}'
  (Should resemble)!

With a super-useful diff:
asdf.T{string:(*string)("barfoo"), s:(*string)("foobar")}

In more complex structs (or structs-of-structs), it is similarly useful. WDYT?

Implement "DeepRender", a method that uses reflection to render a
type's contents recursively to string. Route Should[Not]Resemble calls
through DeepRender to provide insight into types and internals.
@mdwhatcott
Copy link
Contributor

Well, this is pretty amazing! This was probably a lot of work. We've definitely felt the pain you felt, but haven't been willing to put in the effort necessary to fix it.

Just a quick question about the BSD-style license from the Chromium authors you've included with the new files. While the LICENSE.md file in this repo does allow for components of the project to have differing licenses, the main License is MIT (and our license file is LICENSE.md not just LICENSE). So, did this code come from another project?

@danjacques
Copy link
Author

I did write it as part of a Chromium support project. I'll check, but I suspect I can get this resolved. My apologies for the trouble, I'm fairly new to GitHub contributions.

@mdwhatcott
Copy link
Contributor

Hey @danjacques - Just curious if you've made any progress on the licensing concerns for this pull request. Ideally we'd be able to use the license provided with this project. If that isn't possible, we'll need to see the exact LICENSE file your copyright notice refers too, and possibly include it in this project.

I admit I've run into many scenarios lately where this code would be super helpful so I'm hopeful we can work this out. Thanks again for your efforts.

@danjacques
Copy link
Author

Heya! Sorry I meant to follow up over the holiday and completely dropped the ball.

I've refactored my render code out of its original repository and into its own first-class project. This gives you the option of either embedding it (like other internal assertions packages) or making it a go get external. This repository still has the Chromium BSD-style license under which it was developed.

I did acquire permission to relicense it as MIT and embed it as per my original CL (if you're okay adding Google as a copyright holder in LICENSE), but externalizing it in one of the aforementioned ways seemed preferable to me at first glance. Thoughts?

@mdwhatcott
Copy link
Contributor

Ok, I think I will embed/vendor it like I've done with the other helpers. I'll be sure to credit you and link to your github project. Stay tuned. Thanks!

@mdwhatcott mdwhatcott closed this Feb 1, 2016
@mdwhatcott
Copy link
Contributor

Done: 443d812

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.

2 participants