Fix duration measurement on TEST_ORDERED and add unit tests for duration#231
Merged
sagatowski merged 6 commits intoAug 5, 2024
Merged
Conversation
Member
|
@dfreiberger Thanks for this. I tried to run the TcUnit-Verifier but I got this: The relevant lines I guess are: And complete output, the complete test results from TwinCAT looks like this on my node (see below). |
Member
|
@dfreiberger Re-ran the tests three times, same result. |
Contributor
Author
|
@sagatowski I will try again from a new VM when I get an opportunity. What version of TwinCAT and Visual Studio are you running? |
Member
|
Great @dfreiberger, appreciate it! I was running TwinCAT 3.1.4024.55 (latest) and 2019 community (as according to the contributions-doc). |
added 2 commits
May 4, 2024 17:24
…freiberger/fix-duration-on-ordered-tests
Contributor
Author
|
@sagatowski I finally had a chance to look at this again. I had added unit tests to check that the duration value output in the test results was within an expected tolerance range, to catch the error that this PR fixes with the duration calculation for ordered tests (such that it should assert for Do you mind trying this once more? |
Member
|
@dfreiberger Also, looking at your output in your comment: This looks completely crazy? Running the tests locally on my machine I get the following: and: |
This was referenced Jul 28, 2024
Contributor
Author
|
@sagatowski I am not sure what is going on but I will try to remove the unit tests, the bug fix itself is pretty simple, I was just trying to add tests around it to catch the condition but it looks like those tests are not repeatable across environments. Edit: I did try to re-run with the above commits and this is still passing on my system, TwinCAT 3.1.4024.53, Visual Studio 2019, against both Usermode Runtime and a C6017-0010 target.
|
Member
|
@dfreiberger So I've ran this on two different machines now with same result. |
Contributor
Author
|
@sagatowski I was able to reproduce the same error you are seeing on a new VM. It looks like the system I did development on receives the assertion messages in chronological order, whereas the VM receives them out of order, and in my test I was expecting them to be in order. I think this is fixable by sorting the message results by the timestamp they contain and I will look into this. var results = _errors
.Select((e, index) => new { Error = e, Index = index })
.Where(item =>
item.Error.Description.Contains($"| Test name={testName}".ToUpper()) &&
item.Error.ErrorLevel.Equals(vsBuildErrorLevel.vsBuildErrorLevelLow))
.Select(item => _errors.Skip(item.Index).Take(3))
.FirstOrDefault();
if (!results.ElementAt(1).Description.Contains($"| Test class name={className}".ToUpper()))
{
log.Info($"{errorMessagePrefix} does not list class name: {className}");
return;
} |
…rder of the results work
Contributor
Author
|
I added code to sort chronologically, however now the tests are again failing because the duration returned by TcUnit is outside of the range that I am checking for it to be in. I tested against the UserMode runtime and the tests fail I tested against a C6017-0010 and the tests pass I think this indicates that the UM runtime won't be able to calculate duration correctly since it doesn't run deterministically. The timing is calculated by the delta from @sagatowski how do you think we should handle this? To be clear, this is using the duration returned by TcUnit. |
Member
|
@dfreiberger I get the issue even on a virtual machine on my computer. Have not tested it using UM-RT. Running this on a VM on my laptop, I get: And if I look into the detailed result I get:
For now I'm fine with adding a print that running the test-software does not work on the UM-RT but requires the real RT-kernel. Would however not want to impose a requirement to run a real hardware-PLC to run this. |
Member
|
Tests fixed, it was a problem of invariant culture (Europe vs. USA) for checking doubles. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes a bug with the calculation of duration on TEST_ORDERED tests, and adds TcUnit-Verifier tests to check that the duration is output within a specific range for TEST and TEST_ORDERED tests.
To determine if the duration is calculated, a new assertion is added to TcUnit.Verifier called
AssertContainsResultSetwhich parses out the test result lines and checks that the duration is within a specific range.The acceptable range is based on how long the tests took to run on my system, however there is some variability in the duration and it might also depend on the target hardware. @sagatowski please check if you think this is sufficient or if you would prefer a different method.
Finally, I did not increment the version of TcUnit or TcUnit-Verifier as I wasn't sure how you want to handle this.