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

Dumping table rows parameter in the report #688

Closed
fedepaol opened this issue Jun 8, 2020 · 11 comments
Closed

Dumping table rows parameter in the report #688

fedepaol opened this issue Jun 8, 2020 · 11 comments

Comments

@fedepaol
Copy link
Contributor

fedepaol commented Jun 8, 2020

Hi there, we have a matrix - generated suite of tests, which end up as different table rows of a table.
What we would like to do is to have the different parameters dumped in the test output, shall it be the one on standard output or the junit report.

What is the suggested way to have such behaviour? Is dumping the parameters in the entry description the only way?
Would it make sense to (optionally) dump out the parameters in case of table tests? After all, the parameters are part of what's being tested in that particular row. Is this something you would accept as a change?

@blgm
Copy link
Collaborator

blgm commented Jun 8, 2020

Hi @fedepaol. I'm assuming that you're talking about DescribeTable(). Here are some ways that people commonly log more details about their test:

I can see the appeal of always logging the parameters in a DescribeTable(), but I also see some cases where it might be hard to format them. For instance if testing subtle differences in large structs it could be hard to see what's different. If an input parameter is a function then what would you log? My initial reaction is that Ginkgo should give test writers the ability to easily log whatever they like, and should not make assumptions about what a test will want to log. However I am open to being persuaded otherwise.

@fedepaol
Copy link
Contributor Author

fedepaol commented Jun 8, 2020

Thanks @blgm for replying. Yes, I am talking about DescribeTable and the various Entry parameters.

Our need is to be able to dump the parameters in some way, because they are part of the single row description. We may even want to focus / skip some of them.
We started using GinkgoWriter, but the solution we came with felt a bit clunky because of the mandatory lines we needed to add to the tests (plus, by doing that we can't use the parameters for skipping / focusing only on some tests), hence the request for discussion.

On one side, the points you are making make a lot of sense (func parameters, structs to big to log), on the other hand it still feels that something is missing because a given parameter is still describing the specific test that entry is referring to.
When a test fail, I would like to know which parameters it failed with, and there's currently no "native way" if not injecting them in the entry description when I create the entries.
But if you think there can't be an (even optional) way to change that, I'll live with it :-)

Thanks for the help

@blgm
Copy link
Collaborator

blgm commented Jun 8, 2020

I think it's always worth having a discussion.

The place that I'm coming from is that Ginkgo is shared by many projects, and it already quite large and difficult to maintain. Also it's often much harder to write code to solve problems in the general case than is is to solve problems for a specific case. But it's possible that this functionality would be useful to lots of people.

To get the discussion going, I think you're trying to do something like this:

var _ = Describe("test", func() {
	makeEntry := func(input, expected int) TableEntry {
		return Entry(
			fmt.Sprintf("%d squared should be %d", input, expected),
			input,
			expected,
		)
	}

	DescribeTable(
		"squares",
		func(input, expected int) {
			Expect(input * input).To(Equal(expected))
		},
		makeEntry(1, 1),
		makeEntry(2, 4),
		makeEntry(3, 9),
		makeEntry(4, 16),
	)
})

Which gives the output (simplified):

    3 squared should be 8 [It]

    Expected
        <int>: 9
    to equal
        <int>: 8

Of course I'm probably completely wrong, so I'd be really interested to hear more about what you are trying to do.

@onsi
Copy link
Owner

onsi commented Jun 8, 2020

It's UGLY but I could imagine doing this right here:
https://github.com/onsi/ginkgo/blob/master/extensions/table/table_entry.go#L45

by replacing the global.Failer Ginkgo fail handler just before invoking itBody.Call with a fail handler that appends the passed in parameters to the message.

again, it's ugly but would work and have the desired effect. Alternatively, we could add an optional "onFailText" to the It nodes that the table entries can write to and then teach Ginkgo to emit that text on failure as well (not hard, but a little complex). That would be cleaner and would have the added benefit (if we added something to the public interface) of allowing users to annotate their Its with failure messages.

I can try to find some time to work on either approach. Or if you want to take a crack at it @fedepaol feel free to.

@fedepaol
Copy link
Contributor Author

fedepaol commented Jun 8, 2020

I think it's always worth having a discussion.

The place that I'm coming from is that Ginkgo is shared by many projects, and it already quite large and difficult to maintain. Also it's often much harder to write code to solve problems in the general case than is is to solve problems for a specific case. But it's possible that this functionality would be useful to lots of people.

To get the discussion going, I think you're trying to do something like this:

var _ = Describe("test", func() {
	makeEntry := func(input, expected int) TableEntry {
		return Entry(
			fmt.Sprintf("%d squared should be %d", input, expected),
			input,
			expected,
		)
	}

	DescribeTable(
		"squares",
		func(input, expected int) {
			Expect(input * input).To(Equal(expected))
		},
		makeEntry(1, 1),
		makeEntry(2, 4),
		makeEntry(3, 9),
		makeEntry(4, 16),
	)
})

Which gives the output (simplified):

    3 squared should be 8 [It]

    Expected
        <int>: 9
    to equal
        <int>: 8

Of course I'm probably completely wrong, so I'd be really interested to hear more about what you are trying to do.

You got to the point! By having it in the description, we would benefit also of seeing the parameters in the passing tests and we would be able to focus / skip.

@fedepaol
Copy link
Contributor Author

fedepaol commented Jun 8, 2020

It's UGLY but I could imagine doing this right here:
https://github.com/onsi/ginkgo/blob/master/extensions/table/table_entry.go#L45

by replacing the global.Failer Ginkgo fail handler just before invoking itBody.Call with a fail handler that appends the passed in parameters to the message.

again, it's ugly but would work and have the desired effect. Alternatively, we could add an optional "onFailText" to the It nodes that the table entries can write to and then teach Ginkgo to emit that text on failure as well (not hard, but a little complex). That would be cleaner and would have the added benefit (if we added something to the public interface) of allowing users to annotate their Its with failure messages.

I can try to find some time to work on either approach. Or if you want to take a crack at it @fedepaol feel free to.

As per the first approach, I was thinking that maybe (and optionally) we could craft a description containing the params (and maybe in some structured way), here https://github.com/onsi/ginkgo/blob/master/extensions/table/table_entry.go#L50
I would be interested in seeing the params in both success / fail, for the reasons mentioned in the first comment. If that sounds good, I can try to open a pr.

@onsi
Copy link
Owner

onsi commented Jun 8, 2020

Durp - of course, just putting them into the description of the It itself will be straightforward and allow for focus/skip.

One suggestion - what if rather than have Ginkgo try to figure out how to render the description we replace the signature for Entry from:

func Entry(description string, parameters ...interface{}) TableEntry

to

func Entry(description interface{}, parameters ...interface{}) TableEntry

and allow the user to either pass in a string (which would run the current behavior) or a function that will be passed parameters and returns a string. That would give the user control over how to format the string. Thoughts?

@fedepaol
Copy link
Contributor Author

fedepaol commented Jun 8, 2020

func Entry(description string, parameters ...interface{}) TableEntry

to

func Entry(description interface{}, parameters ...interface{}) TableEntry

and allow the user to either pass in a string (which would run the current behavior) or a function that will be passed parameters and returns a string. That would give the user control over how to format the string. Thoughts?

Having a function would have been my next proposal. I am not 100% sure about having it behaving in a "magic" way, mostly because it would be hard for users to discover it from the api, as opposed to having another version of Entry that accepts a function instead of a string.

@onsi
Copy link
Owner

onsi commented Jun 8, 2020 via email

@fedepaol
Copy link
Contributor Author

fedepaol commented Jun 8, 2020

Another version works for me and avoids the magic. There's a lot of magic in Ginkgo - perhaps avoiding even more is a good idea. So - a new version of [FXP]Entry that accepts a function? SGTM!

Deal! Expect a PR soon, and thanks!

@onsi
Copy link
Owner

onsi commented Jun 12, 2020

This is now in onsi/ginkgo@v1.13.0

@onsi onsi closed this as completed Jun 12, 2020
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

No branches or pull requests

3 participants