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

Add a version of table.Entry that allows dumping the entry parameters. #689

Merged
merged 1 commit into from
Jun 9, 2020

Conversation

fedepaol
Copy link
Contributor

@fedepaol fedepaol commented Jun 8, 2020

Sometimes, it's useful to have the parameters of a given table entry as part of the description of the It counterpart.
To achieve that, we add a new family of Entry constructor that accept a function instead of the description as a string.
That function needs to be implemented by the client of the library, and it's called with the list of arguments and returns a string that will be used as description of the given entry.

This was discussed in #688

I am not totally happy with the naming, but I guess keeping it short-ish has a value here.

@fedepaol fedepaol force-pushed the configurableentryoutput branch from eefb527 to a0905ed Compare June 8, 2020 22:12
@onsi
Copy link
Owner

onsi commented Jun 8, 2020

Thanks @fedepaol - this is in the right direction but now I'm realizing why we might actually want to use some magic. The current implementation is not symmetric with the existing implementation of DescribeTable. Specifically, the function passed to DescribeTable has a signature that describes the inputs of the test (e.g. from the test you've added: func(x int, y int, expected bool) however the function passed to DEntry will be forced to have a less descriptive signature: func(params []interface{}) string that will force the user to refer to the parameters by by index instead of by name.

So I think we actually want DEntry to take interface{} (just like DescribeTable does) and allow the user to pass a function that more closely parallels their DescribeTable function.

At that point one could argue that we don't need DEntry and can just go with having Entry do the right thing depending on the type of input it has... but I don't feel too strongly about that.

WDYT?

@fedepaol fedepaol force-pushed the configurableentryoutput branch from a0905ed to bce59f1 Compare June 8, 2020 23:33
@fedepaol
Copy link
Contributor Author

fedepaol commented Jun 8, 2020

The new version is a bit more complex on code, but I do agree, it looks a bit cleaner on client site.
Please take a look.

@onsi
Copy link
Owner

onsi commented Jun 9, 2020

This looks great, @fedepaol - I think all it needs now is to be documented (inline in the comments, but also on the gh-pages branch here: https://github.com/onsi/ginkgo/blob/gh-pages/index.md#table-driven-tests )

If you're up for adding some documentation let me know. Otherwise, I'll pull this in and document it myself.

@fedepaol
Copy link
Contributor Author

fedepaol commented Jun 9, 2020

This looks great, @fedepaol - I think all it needs now is to be documented (inline in the comments, but also on the gh-pages branch here: https://github.com/onsi/ginkgo/blob/gh-pages/index.md#table-driven-tests )

If you're up for adding some documentation let me know. Otherwise, I'll pull this in and document it myself.

Sure, I'll do! TBH, it was pretty late yesterday and I wasn't able to find the docs :P

Sometimes, it's useful to have the parameters of a given table entry as part of the description of the It counterpart.
To achieve that, we allow passing a function as the description parameter.
In case a string is passed, the previous behaviour is preserved. In case of function, it is expected to return a string
that will be used as the new description for the generated It.
@fedepaol
Copy link
Contributor Author

fedepaol commented Jun 9, 2020

@onsi done! Feel free to correct it.

@onsi
Copy link
Owner

onsi commented Jun 9, 2020

Looks great! I’ll merge these in when i get to my desk.

@onsi onsi merged commit 21eaef2 into onsi:master Jun 9, 2020
@fedepaol
Copy link
Contributor Author

fedepaol commented Jun 9, 2020

Thanks! When do you expect this to be available as a tag?

@onsi
Copy link
Owner

onsi commented Jun 9, 2020

I expect in the next couple of days or so

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