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

Parse test execution time in unity fixture output #659

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

fy666
Copy link

@fy666 fy666 commented Feb 14, 2023

When using unity fixture with time output flag set, parse_output.rb did not parse the execution time.

@fy666
Copy link
Author

fy666 commented May 16, 2023

Hello @mvandervoord,

I'm very sorry to bother you but I was wondering if I did something wrong or I missed some steps for submitting this PR (as I got no reaction) ?

Best regards,

@@ -131,10 +131,11 @@ def prepare_fixture_line(line)
def test_passed_unity_fixture(array)
class_name = array[0]
test_name = array[1]
test_time = get_test_time(array[array.length - 1])
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this needs to be enabled by something, as by default we do not have the timing information.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for reviewing :)
Yes indeed, the timing output has to be enabled using UNITY_INCLUDE_EXEC_TIME flag

Copy link
Contributor

Choose a reason for hiding this comment

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

so shouldnt there be an if statement, otherwise you are just accessing test_name in the worst case?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, if the execution time is not present get_test_time will receive a string that wont match the "( ms)" pattern and return 0 (that strategy was already applied for normal unity output)

@Letme
Copy link
Contributor

Letme commented May 16, 2023

Why is this printout only for passed and not for other cases (failed)?

@Letme
Copy link
Contributor

Letme commented May 16, 2023

in line 212 failed case also has time, so I am a bit puzzled why this also does not have it (as well as ignored).

@fy666
Copy link
Author

fy666 commented May 17, 2023

in line 212 failed case also has time, so I am a bit puzzled why this also does not have it (as well as ignored).

I updated the PR to parse time for fail and ignore cases (for consistency)

However, please note that in unity fixture, the execution time is only printed for passing tests: https://github.com/ThrowTheSwitch/Unity/blob/master/extras/fixture/src/unity_fixture.c#LL298C13-L298C34
I can update that as well if you want

@Letme
Copy link
Contributor

Letme commented May 17, 2023

However, please note that in unity fixture, the execution time is only printed for passing tests:
I can update that as well if you want

This I can't decide, but I like uniform behavior among features. Strange that other outputs had time in as well - so maybe that is inconsistency which we need to fix. Either have execution time on passing testcases only, or on all (but everywhere the same).

@fy666
Copy link
Author

fy666 commented Jun 21, 2023

However, please note that in unity fixture, the execution time is only printed for passing tests:
I can update that as well if you want

This I can't decide, but I like uniform behavior among features. Strange that other outputs had time in as well - so maybe that is inconsistency which we need to fix. Either have execution time on passing testcases only, or on all (but everywhere the same).

Sorry for the delay.
I just added a commit to make unity fixture output the execution time for failed and ignored tests as well.

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.

2 participants