Skip to content

Fix Ginkgo stack trace on failure for Specify#415

Merged
jvshahid merged 1 commit into
onsi:masterfrom
williammartin:specify-stack
Dec 21, 2017
Merged

Fix Ginkgo stack trace on failure for Specify#415
jvshahid merged 1 commit into
onsi:masterfrom
williammartin:specify-stack

Conversation

@williammartin

@williammartin williammartin commented Dec 19, 2017

Copy link
Copy Markdown
Collaborator

This changes the stack trace printed on failure to point at the failing test like It does, instead of into ginkgo_dsl.

Before:

/Users/pivotal/go/src/github.com/williammartin/ginkgoleaktest/ginkgoleaktest_test.go:8
  doesn't print the right stack trace [It]
  /Users/pivotal/go/src/github.com/onsi/ginkgo/ginkgo_dsl.go:365

  fail the test

  /Users/pivotal/go/src/github.com/williammartin/ginkgoleaktest/ginkgoleaktest_test.go:11

After:

Ginkgoleaktest
/Users/pivotal/go/src/github.com/williammartin/ginkgoleaktest/ginkgoleaktest_test.go:8
  doesn't print the right stack trace [It]
  /Users/pivotal/go/src/github.com/williammartin/ginkgoleaktest/ginkgoleaktest_test.go:10

  fail the test

  /Users/pivotal/go/src/github.com/williammartin/ginkgoleaktest/ginkgoleaktest_test.go:11

The diff is in the line after [It]. There might be a slightly more DRY way to do this by providing a utility method that pushes a node with an injectable codelocation, but I don't think it really makes things simpler.

[fixes #414]

@williammartin williammartin mentioned this pull request Dec 19, 2017
Merged
@iandelahorne

iandelahorne commented Dec 19, 2017

Copy link
Copy Markdown
Collaborator

LGTM 👍

jvshahid
jvshahid requested changes Dec 19, 2017
View reviewed changes

@jvshahid jvshahid left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like something that we can test in the integration test, e.g. https://github.com/onsi/ginkgo/blob/master/integration/fail_test.go

I'm happy to write the test if you can't get it to it faster

@williammartin

williammartin commented Dec 19, 2017

Copy link
Copy Markdown
Collaborator Author

Like it @jvshahid Maybe can get to it tomorrow. Won't be today.

@jvshahid jvshahid merged commit b977ede into onsi:master Dec 21, 2017
jvshahid added a commit that referenced this pull request Dec 21, 2017
@jvshahid
add a test case for #415
65ca40e
jvshahid
jvshahid reviewed Dec 21, 2017
View reviewed changes

@jvshahid jvshahid left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test was added in 65ca40e

jvshahid added a commit that referenced this pull request Dec 21, 2017
@jvshahid
fix the tests on go versions <1.9
6c46eb8 
fixes #415
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jvshahid jvshahid jvshahid requested changes

@iandelahorne iandelahorne Awaiting requested review from iandelahorne

Assignees

No one assigned

Labels

bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Specify prints an unpleasant stack trace on failure

3 participants

@williammartin @iandelahorne @jvshahid