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

Consider returning boolean for Asserts #124

Open
slacAWallace opened this issue Nov 16, 2020 · 12 comments
Open

Consider returning boolean for Asserts #124

slacAWallace opened this issue Nov 16, 2020 · 12 comments
Labels
enhancement New feature or request

Comments

@slacAWallace
Copy link
Contributor

I am finding myself wishing for some kind of result from an assert method to tell me if the assertion passed/failed. Some of my test setups use a loop and if one assertion fails there is little to no point in running through the rest.

@sagatowski
Copy link
Member

Hi @slacAWallace & thanks for this great suggestion! Although I think your suggestion makes a lot of sense (I too have tests that can take a lot of time to finish), I'm not sure how this should work. Or at least it raises some questions. Just a few questions to initiate the discussion:

  1. Should the default behaviour be to abort running of tests if an assertion fails?
  2. Maybe it's enough to know that a test has failed? If you can check that a test has failed during running, you can yourself decide whether the rest of the tests should continue to run or not? There is a function in the framework right now to check if a test has finished (IS_TEST_FINISHED()), maybe something like TEST_STATUS() could be added that returns an enumeration (FAILED, SUCCESSFUL, RUNNING)?

Also, it would be interesting to see how other unit testing frameworks handle this (if they do at all).

Regarding (1) I think the default behaviour should always be to finish all tests independently of whether one test fails or not. An option could be to add a parameter to the TcUnit-framework to enable a feature that stops running of the tests if one test fails. By using parameters, the user could configure each use of TcUnit for every library as the parameters for library references are stored in the project itself.

@sagatowski sagatowski added the enhancement New feature or request label Nov 22, 2020
@sagatowski
Copy link
Member

@slacAWallace Would it be a good solution/option for you to have a parameter that you can set (through the GVL_Param_TcUnit), that if it is set, if an assertion fails, the rest of the tests are skipped (not enabled by default)?

@kumaraswamygaviyappa
Copy link

@slacAWallace Thank you for this question. I have a requirement as mentioned below.

test := AssertEquals_BOOL
IF test THEN
    ADSLOGSTR(
        msgCtrlMask := ADSLOG_MSGTYPE_HINT, 
        msgFmtStr   := 'Test message. Parameter is %s', 
        strArg      := 'ABC'
    );      
END_IF

Watching this space for the solution.

@RodneyRichardson
Copy link

RodneyRichardson commented Feb 22, 2021

Many unit testing frameworks will raise a specific exception within ASSERT to prevent a test from continuing, but to still execute other tests. I would like to see this added, although it would likely be a breaking change and may require v2.0. It could possibly be added as an option controlled by a BOOL parameter for v1.3?

Edit: It looks like exception handling (__TRY/__CATCH) is only supported on 32-bit systems at the moment, so this may not be practical yet.

@sagatowski
Copy link
Member

@kumaraswamygaviyappa All Assert-function provides a message-field, so that you can enter your own message if an assertion should fail. If an assertion fails, the message will be displayed.

@RodneyRichardson One option could be to add a parameter to TcUnit, to stop running the tests if one assertion fails, though I'm not sure this is what @slacAWallace wants?

@slacAWallace
Copy link
Contributor Author

@RodneyRichardson One option could be to add a parameter to TcUnit, to stop running the tests if one assertion fails, though I'm not sure this is what @slacAWallace wants?

That's right, I am hoping to get something a bit more local. I have some test patterns I use that keep a test open for multiple cycles, until a particular condition is met (a counter value for example). If in the middle of these tests an assert fails, then there's often no reason to keep running through the rest of the asserts. I'll try to sketch this up soon with a PR.

@sagatowski sagatowski added this to the 1.3.0.0 milestone Mar 6, 2021
@sagatowski
Copy link
Member

sagatowski commented Mar 6, 2021

Moved this to Release 1.3.0.0.
Looking forward to your PR @slacAWallace (don't forget to read the CONTRIBUTING document).

@slacAWallace
Copy link
Contributor Author

I sat down over the last weekend to prepare something for this and ended up just thinking. After some discussion with a colleague yesterday I think I understand @sagatowski 's original response more completely now.

  1. Changing tcunit to mark a test finished at the first failed assert will make it more like python test frameworks at the very least. Whether this is appropriate for a PLC unit testing system is another question. I would err on the side of doing it this way.
  2. If the change in 1 is made, then I would change my test structures to use the IS_TEST_FINISHED function to branch accordingly.

Should I prepare a PR to investigate the change in behavior?

@HAHermsen
Copy link

HAHermsen commented Oct 6, 2021

On 1) I could imagine that this behaviour could be enabled or disabled by the user. I.e. some global variable to either continue testing after a fail or stop testing after a fail. If you keep it testing after fail this defaults to current behaviour.

Als could you see if you can easily implement some way to reset the framework and restart the tests without a new download or cold reset of the PLC? I'd love to see a second opinion on this!?

Thank you in advance.

Edit:
PS The Parameterlist is phased out from CODESYS since SP16 or so. Usage is still possible but the meantime no adequate alternative is given so we must stick with it for now. An alternative will come in SP18 but that will take at least a small year and thus even a bit longer for TwinCAT to follow up.

@sagatowski
Copy link
Member

sagatowski commented Oct 7, 2021

@slacAWallace You're welcome to do it, but take into consideration this issue. If this change is done, you could instead simply do

IF TEST('Blahonga') THEN
// Only execute FB under test and do assertions here
END_IF

The IF would be optional, not to break any existing tests to be 100% backward compatible with tests written in prior versions of TcUnit.
I haven't dig deeper into the above issue yet though, and haven't investigated whether this would truly work without breaking any existing usages.

Also, don't forget CONTRIBUTING.

@HAHermsen
Why were ParameterList removed (without having any alterantive)? They are quite commonly used.
You can implement the suggested function in CfUnit, and if you find it works well we can back-port it into TcUnit.

@HAHermsen
Copy link

HAHermsen commented Oct 7, 2021

@HAHermsen
Why were ParameterList removed (without having any alternative) They are quite commonly used =>
So True! I was kind of surprised when I found out they are not supported anymore. CODESYS decided to "silently drop" them from SP16 onwards (we are at 17 now). The exact reason is unknown to me. For now they are still usable in CODESYS projects but we cannot insert them in a new project, since the object has disappeared from the add object list dialog. A workaround is to export a Parameter List from an older IDE/project and re-import it into a new project.
I was told that a alternative will follow suit but we must wait for SP18 for it. As soon as I receive more info, I'll drop a message here.

EDIT
PS cfunit has changed name to co♻e: A unittest framework for CODESYS (in short coUnit), because of obvious reasons (co = codesys, tc = twincat). I only wished we came up with this name a year earlier ;-)

PSS
We also use git now since the launch of SP17, CODESYS IDE has a (paid) git client. Its fairly recent so not as feature rich yet but it manages quite nice.

@sagatowski
Copy link
Member

No activity in a while. I'll move this as an idea for TcUnit 2.0, as I've seen some projects using asserts in such a way that they DON'T expect a boolean to return as this would break their static code analysis.
For anyone that comes across this and needs an urgent solution, you can call IS_TEST_FINISHED() in your state-machine when you are doing your assert (if you have multiple asserts, which I assume is the case here). If this returns true, simply don't do any more asserts. It's not as clean solution as getting a boolean directly, but it will work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants