search

tcunit / TcUnit-Runner

Program that makes it possible to automate runs of TcUnit unit tests
Other
34 stars 17 forks source link

Implement timeout (or react to exceptions), solves issue #10 #13

Closed stefanbesler closed 3 years ago

stefanbesler commented 3 years ago

This pull requests solves the issue that is mentioned in issue #10 .

An additional option to specify a timeout for the process is implemented. I observed that the automation interfaces does not perform particularly well on system with low RAM. Even though a potential fix was provided in issue #5 , the problem still occurs from time to time. If a timeout is provided, a user of TcUnit-Runner can implement a retry(x) {} step in Jenkins to keep the butler happy.

During testing we are now monitoring the AdsState for all PLC ports that are used in the solution. If any of them is not AdsState.Run the process is terminated. TcUnit-Runner prepares an environment where the AdsState should be AdsState.Run and if it is not the case, this is an indication for a TwinCAT issue (no license activated, PLC Exception).

I tested the implementation with my setup, but more testing on your side, @sagatowski would be appreciated.

sagatowski commented 3 years ago

@stefanbesler My plan is to do a new release before end of next week. Do you think you have time to fix these (minor) comments, or do you want me to merge your stuff and do the final fixes? If you don't have time, it's no hurry, I can wait!

stefanbesler commented 3 years ago

I think I can finish this by tomorrow evening.

Am 21.12.2020 um 11:36 schrieb Jakob Sagatowski notifications@github.com:

 @stefanbesler My plan is to do a new release before end of next week. Do you think you have time to fix these (minor) comments, or do you want me to merge your stuff and do the final fixes? If you don't have time, it's no hurry, I can wait!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

sagatowski commented 3 years ago

@stefanbesler Thanks much appreciated! I've done some commits for other stuff recently as well, I hope you can merge these changes into your PR. If not, I can do the merge on my side.

stefanbesler commented 3 years ago

@sagatowski no worries, I merged it.

sagatowski commented 3 years ago

@sagatowski no worries, I merged it.

  • There are two issues you mentioned in your review that I tried to explain, but didnt change any code. You didn't react to my comments so please check again.

I've watched the comments and the two I've found I've responded to both. Have I missed something?

  • I don't know if you want me to modify the bat file as well. I don't really use this bat script on my Jenkins servers and prefer calling the exe directly. Timeout is also a rather specific parameter and that's why I didnt add it to the bat file.

I'll do the modification of the bat-file.

  • During the merge I noticed the new parameter to force the Twincat version. For consistency I'd suggest to rename it to TcVersion (as it is also called TcUnitTaskName)

Makes total sense, I'll change it!

I'll start the merge during the day!

stefanbesler commented 3 years ago

@sagatowski i mean these comments, I do not know of you expect any fix here or if my explanations are ok for you to resolve this issues

@sagatowski sagatowski 6 days ago Member I don't have so much experience with this, but how does this AdsClient work. I mean, I guess >one assumption is that the machine where this is running needs to have an active AMS >connection to the target? Is there a DLL for ADS that follows along TcUnit-Runner... ?

@stefanbesler stefanbesler Author Contributor Pending Yes, you are right, a route is to be set up. Local targets ofc do not. If the host does not answer, an Exception is thrown and caught here (see comment of the try block). The method >>does not block forever, but times out. However, there is no parameter to specify the timeout. I believe the setting for the timeout is taken from the AMS router the route needs to be set up anyway for activating on the target and I think it is mentioned clearly in the comment above the try block, that if we can't get the state for some reason, we simply continue. Do you expect something else here?

And this one

@sagatowski sagatowski 6 days ago Member Is this blocking? What happens if we don't manage to read the state?

@stefanbesler stefanbesler Author Contributor Pending I never had issues with reading the state if the client could connect, and exceptions during connect are caught. is this ok, or is this still bothering you?

sagatowski commented 3 years ago

@sagatowski i mean these comments, I do not know of you expect any fix here or if my explanations are ok for you to resolve this issues

@sagatowski sagatowski 6 days ago Member I don't have so much experience with this, but how does this AdsClient work. I mean, I guess >one assumption is that the machine where this is running needs to have an active AMS >connection to the target? Is there a DLL for ADS that follows along TcUnit-Runner... ?

@stefanbesler stefanbesler Author Contributor Pending Yes, you are right, a route is to be set up. Local targets ofc do not. If the host does not answer, an Exception is thrown and caught here (see comment of the try block). The method >>does not block forever, but times out. However, there is no parameter to specify the timeout. I believe the setting for the timeout is taken from the AMS router the route needs to be set up anyway for activating on the target and I think it is mentioned clearly in the comment above the try block, that if we can't get the state for some reason, we simply continue. Do you expect something else here?

Aha! All good!

And this one

@sagatowski sagatowski 6 days ago Member Is this blocking? What happens if we don't manage to read the state?

@stefanbesler stefanbesler Author Contributor Pending I never had issues with reading the state if the client could connect, and exceptions during connect are caught. is this ok, or is this still bothering you?

No, this is fine! Thanks for comments!

I just have a tiny problem, and that is that I can't compile the project for some reason. I get this error: image

Am I missing something basic?

stefanbesler commented 3 years ago

Oh sorry, this is probably not supported in VS2013. Please rewrite it with normal string concatenation.

log.Info(„Timeout enabled - process times out after “ + Timeout + „ minutes“);

sagatowski commented 3 years ago

Aha, I see! Will do!

sagatowski commented 3 years ago

Merged into main. Did this additional changes on top of this:

Tested with various pinned/non-pinned projects of TwinCAT 3.1.4022.30 and 3.1.4024.11.

@stefanbesler Thanks for your contribution!