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

test/cli/test-other.py: Fix test_showtime_top5_file. #5847

Merged
merged 1 commit into from
Jan 6, 2024

Conversation

mvds00
Copy link
Contributor

@mvds00 mvds00 commented Jan 6, 2024

test_showtime_top5_file() assumes that all reported elements, of which the "top 5" are validated, end with the string "- 1 result(s))".

This is clearly not the case when viewing the entire list:

$ cppcheck --showtime=summary --quiet empty.c  |grep "2 result"
valueFlowLifetime(tokenlist, errorLogger, settings): 3.4e-05s (avg. 1.7e-05s - 2 result(s))
valueFlowEnumValue(symboldatabase, settings): 3e-06s (avg. 1.5e-06s - 2 result(s))

As the order of items is non-deterministic, this test makes CI workflows randomly fail.

This patch addresses the issue by adjusting the expected string to the reported item.

test_showtime_top5_file() assumes that all reported elements, of which the "top
5" are validated, end with the string "- 1 result(s))".

This is clearly not the case when viewing the entire list:

```bash
$ cppcheck --showtime=summary --quiet empty.c  |grep "2 result"
valueFlowLifetime(tokenlist, errorLogger, settings): 3.4e-05s (avg. 1.7e-05s - 2 result(s))
valueFlowEnumValue(symboldatabase, settings): 3e-06s (avg. 1.5e-06s - 2 result(s))
```

As the order of items is non-deterministic, this test makes CI workflows
randomly fail.

This patch addresses the issue by adjusting the expected string to the reported
item.
@firewave
Copy link
Collaborator

firewave commented Jan 6, 2024

This is a known issue: https://trac.cppcheck.net/ticket/12106.

I did not address it yet since I am not sure what would be actually be expected. The calls should possibly have unique identifiers so you can differentiate them otherwise the output might not be helpful. It's hard to tell as I never have used it to identify performance issues.

@mvds00
Copy link
Contributor Author

mvds00 commented Jan 6, 2024

This is a known issue: https://trac.cppcheck.net/ticket/12106.

I did not address it yet since I am not sure what would be actually be expected. The calls should possibly have unique identifiers so you can differentiate them otherwise the output might not be helpful. It's hard to tell as I never have used it to identify performance issues.

Thanks for bringing this to light. I've noticed that there are typically just two valueFlow items that consistently report two results. It would be really helpful to address this, as the current situation can lead to some confusion and random failures in CI workflows, which isn't ideal.

@firewave
Copy link
Collaborator

firewave commented Jan 6, 2024

Thanks for bringing this to light. I've noticed that there are typically just two valueFlow items that consistently report two results.

That's because we execute some valueflow steps are run multiple times in one iteration. I was also looking into collecting separate timing information for each iteration. It's been a while though so I can't really remember all the details.

It would be really helpful to address this, as the current situation can lead to some confusion and random failures in CI workflows, which isn't ideal.

Definitely. Fortunately it doesn't happen that often. This would be fine as a workaround until we have decided if we want/need to change it.

@danmar
Copy link
Owner

danmar commented Jan 6, 2024

I think this solutions is ok for now too..

@danmar danmar merged commit 328f98b into danmar:main Jan 6, 2024
64 checks passed
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