search

tcunit / TcUnit

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

Remove duplicate code #10

Closed sagatowski closed 3 years ago

sagatowski commented 5 years ago

In several of the assert-methods there is quite a lot of code that is duplicated. Clean that up so it's using common functions/function blocks for these parts.

ascaron37 commented 5 years ago

Hi Jakob, for me there are two kinds of duplication:

  1. Assert in the method name: assert.Assert... I would rather have something like assert.int_equal. What do you think?
  2. Do you want to keep the explicit Equals_TYPE checks? How about only one equal method with ANY inputs? I see only one disadvantage that you can not use constants any more. Otherwise it can be an advantage because you increase readability.
sagatowski commented 5 years ago

Hi Maxim!

  1. I'm thinking to keep the assert-name in the method as the user could instantiate the function block FB_Assert with any name they want (for instance, fbA) and then I think it makes sense to keep the name. I'm basically following the naming scheme for JUnit, as this is a unit testing framework a lot of developers recognize: http://junit.sourceforge.net/javadoc/org/junit/Assert.html

  2. There actually already exists an Assert-method that takes two ANYs. I was originally planning to use only the Assert with two ANYs. The problem was that ANY was only introduced quite recently (3.1.4020.0), which means that it only works from that version. The problem is that if a user wants to fork the repository to be able to use TcUnit with 4016 or 4018, they can not use the ANY-version. So these primitive types ones are necessary for backward-compability. If there only existed method overloading in IEC61131-3/Structured text, we wouldn't have this problem.

What I was referring to for this "duplication of code" in this issue is all the "boiler-plate" type of code in every Assert-method, such as the code in AssertArrayEquals_BOOL, where for example theres tons of checks to check if the size of the array is equal. This is done in the same way in all the different versions of the AsserArrayEquals. The only problem I've had with this is that you cannot define arrays of variable size (Array[*]) of the ANY-type, which would be really nice. So I thought I'd create this issue just to remember to try to find a solution for cleaner code.

ascaron37 commented 5 years ago

Alright, for backwards compatibility sakes I get that. But with ReportResult having two ANYs it is going to be harder to fork. I have an idea which would also decrease duplicate code. How is it best to contribute? By creating a fork or feature branches? Sorry, I never worked with PRs.

sagatowski commented 5 years ago

There is an excellent guide available at GitHub: https://help.github.com/articles/creating-a-pull-request-from-a-fork/

I'm writing a contribution-guide for this project, though I'm not done until end of this week, so you can just go ahead! Thanks!

sagatowski commented 5 years ago

In the latest commit i have wrongfully written that I have solved this issue, which is not true. What I meant is that I solved Issue #12. This issue is still open.

sagatowski commented 3 years ago

Not really feasible in a sensible way, so closing this now. If re-created, it should be a discussion of how this could be achieved.