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

Measure overall, testsuite and test duration #159

Merged
merged 14 commits into from
Apr 29, 2023

Conversation

stefanbesler
Copy link
Contributor

@stefanbesler stefanbesler commented Aug 22, 2021

This PR extends to code in a way to fill the "time" attribute of the junit xml specification. I haven't tested this extensively, but just want to let you know that I am on this and maybe you have some opinions and could do a code review on it? I am not completely fluent with TcUnit's structure and maybe there are better ways to include time measurements.

I am not sure if I have enough time at the moment for this as a side project, but in the end I want to extend TcUnit with benchmark capabilities. At the moment, one of my hobby projects is porting a trajectory generator to TwinCAT ( github project: struckig ) and I am using TcUnit for Unittesting . I think it would make sense to use it in a benchmark context as well by including test iterations and stuff like that (similar to what I have done without a library here (https://github.com/stefanbesler/struckig/tree/main/benchmark) ). My goal is to see potential regressions by utilizing a benchmark github action.

@stefanbesler stefanbesler changed the title Measure overall, testsuite and test duration and write into junit Measure overall, testsuite and test duration Aug 22, 2021
@sagatowski
Copy link
Member

sagatowski commented Sep 2, 2021

Hi! Sorry for late reply. Thanks for looking into this. I really like this function. Is it possible that you could use the latest TwinCAT 3.1.4022.x to do the changes though? (according to https://github.com/tcunit/TcUnit/blob/master/CONTRIBUTING.md).

What is the duration measured in? MS?
I guess that if we want to have this included in the JUnit XML-file we will also need to update the TcUnit-Runner.

Did you try to run the TcUnit-Verifier with the new version of TcUnit, just to check that no functionality has been broken?

Edit: Cool initiative with struckig! I might use this myself.

@stefanbesler stefanbesler force-pushed the feature/time_support branch 2 times, most recently from 6902555 to 8c0589b Compare January 11, 2022 16:43
@stefanbesler
Copy link
Contributor Author

stefanbesler commented Jan 11, 2022

so, half a year later ... :-)

Is it possible that you could use the latest TwinCAT 3.1.4022.x to do the changes though?

done, tested it on 4022.29

What is the duration measured in? MS?

it was in microseconds before, but I changed to seconds, which is according to junit's specification or at least how the Jenkins plugins interprets it

I guess that if we want to have this included in the JUnit XML-file we will also need to update the TcUnit-Runner.

Well, I would have prefered to only consider the duration in the xml output of TcUnit, but anyway ... I am on it, I will make a pull request in TcUnit-Runner as well

Did you try to run the TcUnit-Verifier with the new version of TcUnit, just to check that no functionality has been broken?

I ran TcUnit-Verifier, but I had to modify some stuff to get it working (change IDE and Twincat Version that is verified with), would you run it on your setup as well please?

@sagatowski
Copy link
Member

Sorry for late response, been really busy lately...
I will review this, but one thing I noticed is that there are no tests added for this functionality into the TcUnit-Verifier. Is it possible to add a few tests to make sure that you get the expected output?

@stefanbesler
Copy link
Contributor Author

I added and changed some tests for the durations functionality

Copy link
Member

@sagatowski sagatowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First of all, sorry for the long delay of reviewing this PR (as you may know, I'm spending all my spare-time on the TwinCAT YouTube course). Thanks for this awesome PR. I've added some review comments. Could you also look into the comment of @Navid-Mashayekh?

TcUnit/TcUnit/GVLs/GVL_TcUnit.TcGVL Outdated Show resolved Hide resolved
TcUnit/TcUnit/DUTs/ST_TestSuiteResult.TcDUT Outdated Show resolved Hide resolved
TcUnit/TcUnit/POUs/FB_Test.TcPOU Outdated Show resolved Hide resolved
TcUnit/TcUnit/POUs/FB_Test.TcPOU Outdated Show resolved Hide resolved
TcUnit/TcUnit/POUs/FB_TestSuite.TcPOU Outdated Show resolved Hide resolved
TcUnit/TcUnit/POUs/FB_TestSuite.TcPOU Outdated Show resolved Hide resolved
TcUnit/TcUnit/POUs/FB_TestSuite.TcPOU Outdated Show resolved Hide resolved
TcUnit/TcUnit/POUs/Functions/TEST_FINISHED_NAMED.TcPOU Outdated Show resolved Hide resolved
TcUnit/TcUnit/TcUnit.plcproj Show resolved Hide resolved
TcUnit/TcUnit/GVLs/GVL_TcUnit.TcGVL Outdated Show resolved Hide resolved
feat: use getcpucounter instead of getcpuaccount
refactor: use a function for getting the cpucounter instead of directly calling the function block
sagatowski added a commit that referenced this pull request Apr 29, 2023
Corrected parsing from/to float<->string so that Culture.InvariantCulture is used (Swedish for example uses "," instead of "." as comma-delimiter).
@sagatowski sagatowski merged commit 5e6f805 into tcunit:master Apr 29, 2023
@sagatowski
Copy link
Member

Better late than never. I did some minor corrections to your PR. Thank you so much for your contribution @stefanbesler.

@sagatowski
Copy link
Member

@stefanbesler Note that this new feature doesn't work with sequenced tests. Check this issue: #210.
Would you have time to look into it?

@sagatowski
Copy link
Member

@stefanbesler I've also found a second issue with this PR. See #211.

sagatowski added a commit that referenced this pull request May 1, 2023
…sequence" and #211 "Duration doesn't seem to be correctly calculated".

Correction of PR #159 "Measure overall, testsuite and test duration".
@sagatowski
Copy link
Member

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

Successfully merging this pull request may close these issues.

3 participants