sio / LibPQ

Detach your M code from workbooks to reuse it! Import modules from local or web storage (unlimited number of sources)
Apache License 2.0
76 stars 21 forks source link

Added functionality to Assert.Raises for function with arguments #19

Closed estuelke closed 4 years ago

estuelke commented 4 years ago

I found myself in need of testing for an error in a function that takes arguments and made this tweak for my project. It works on non-zero argument functions as well.

Thanks for this project! It's making my Power Queries so much better.

sio commented 4 years ago

Thank you for suggesting an improvement to LibPQ!

The functionality you're looking for has always been there though I admit it was buried pretty deep. I was probably not aware of Function.Invoke at the time I've added Assert[Raises], so I've gone with the next best thing - any function call can be wrapped into a zero-arguments anonymous function. See for example: https://github.com/sio/LibPQ/blob/d8ab696ff3527758e05715ca03441f107263366c/Tests/Tests.PromoteHeadersNonEmpty.pq#L53-L57

I agree that having explicit support for providing arguments is better than the current solution.

Unfortunately, I can not accept your edit because it changes the signature of Assert[Raises] function. That would break existing code for all people who use that function in their unit tests, and would be against the project's values of maintaining backwards compatibility.

Instead I suggest adding a new assertion function that explicitly supports passing arguments to the function being tested. Here is a draft. I'm not sure I like the name though, may be there is a better one?

I've considered

What would you suggest?

sio commented 4 years ago

I've renamed the function to InvokeRaises and pushed changes into master

estuelke commented 4 years ago

Hi Vitaly,

That's great to hear! Sorry I was unable to get back to you until this morning. I like InvokeRaises as a name. Another option, in case you only wanted one Raises function could be changing the signature to include optional args at the end. Though now that I say that, optional detail would have to change to optional detail as nullable and that would cause the same signature change issue. Looks like InvokeRaises is the best option.

Thanks for considering my update!