search

tcunit / TcUnit

An unit testing framework for Beckhoff's TwinCAT 3
Other
273 stars 75 forks source link

Ambiguity of method name TEST_FINISHED_NAME #43

Closed Roald87 closed 5 years ago

Roald87 commented 5 years ago

I tried out the TEST_FINISHED_NAME function and found out that I completely misunderstood its meaning. I thought this function was meant to check if a certain test had finished or not. However, it sets a certain test as finished.

It would be a good idea to change the name to something clearer such as SET_TEST_FINISHED.

Also I made a new function called HAS_TEST_FINISHED, which does what I initially thought the TEST_FINISHED_NAME would do: it checks if a certain test in the current test suite has finished.

I could open up a PR with this new function if you want.

sagatowski commented 5 years ago

We're quite close to release (next week) and I'm a little afraid to do API-changes in the last minute. I think the layout TEST('Name') ... TEST_FINISHED() makes quite good sense! I'll have to think about this...

You're welcome to create a PR and add HAS_TEST_FINISHED(), though I would probably go with IS_TEST_FINISHED as "IS" is the convention used throughout the code. Seems both work equally well though:

https://stackoverflow.com/questions/6597967/net-coding-standards-using-a-prefix-is-or-has-on-method-names

Edit: Make sure to follow: https://github.com/tcunit/TcUnit/blob/master/CONTRIBUTING.md and add tests that verify the new method according to: https://github.com/tcunit/TcUnit-Verifier/blob/master/README.md

Add a separate PR in the TcUnit-Verifier

Roald87 commented 5 years ago

I didn't mean that you would change the current TEST('Name') ... TEST_FINISHED() structure. It also makes sense in my opinion. I was, and still am, confused about the use case of TEST_FINISHED_NAMED('Name').

Initially I thought TEST_FINISHED_NAMED('Name') could be somewhere between TEST('Name') and TEST_FINISHED() to check if a certain (probably previous) test was already finished. However, later I found out you can actually set a certain test to a finished state. Therefore I suggested to rename it to something like SET_TEST_FINISHED('Name') to prevent confusion.

Sure no problem I can change it to IS_TEST_FINISHED('Name'). I thought you used the Has prefix for some methods, but I see that you changed it in the mean time.