util-cpu: add a mockup value for unittests v1#13645
Conversation
Tests were previously platform-dependent, as UtilCpuGetNumProcessorsOnline retrieved the real number of CPU cores, and on systems with fewer than 4 CPU cores, some threading unit tests failed. Ticket: 7831
| #ifdef UNITTESTS | ||
| return 40; // A mockup value for unittests | ||
| #endif |
There was a problem hiding this comment.
How does this affect thing if I build with unittests, but then run live, on a box with 2 cores?
There was a problem hiding this comment.
I was afraid this case should be supported as well 😅 alternatively the tests can check real CPU core presence and skip tests based on the environment and requirements. This is implemented in the other PR#13644
There was a problem hiding this comment.
If it is a usual and still working case, then surely this would not be acceptable, and I would only propose skipping the tests on insufficient machine capabilities.
There was a problem hiding this comment.
I wonder if rather ThreadingAffinityTest03 and such should be skipped if the requirements are not met
There was a problem hiding this comment.
Yes, I would be in favor of that approach as is implemented in #13644 in https://github.com/OISF/suricata/pull/13644/files#diff-1700c8224734e17b7f5fc2cb71b7c53c110438d178cf44ae0d35312c32b3e229R2060
Alternatively, we can add another switch to e.g. configure script that would enable unittest mockups and HW dependent tests would be otherwise skipped? configure --enable-unittests --enable-unittest-mockups
But I still prefer the real HW test as we can generally limit the tests to, e.g., 4 cores (or other limited HW resources).
There was a problem hiding this comment.
So, should you close this PR in favor of #13644 and such ? (if you prefer the other PR as I do)
There was a problem hiding this comment.
Note that this actually breaks Debian package tests which run the unit tests on i386 platforms where the test machines have less than 4 CPUs: https://ci.debian.net/packages/s/suricata/testing/i386/64736439/
I also have a patch which determines at runtime whether UtilCpuGetNumProcessorsOnline() is called from a unit test or not, and which returns a hardcoded mock value in that case (See 994d01c) -- but would also be fine with skipping the tests.
For Debian I would just disable these tests so they don't hold up testing transitions.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #13645 +/- ##
=======================================
Coverage 83.69% 83.69%
=======================================
Files 1011 1011
Lines 275071 275074 +3
=======================================
+ Hits 230210 230226 +16
+ Misses 44861 44848 -13
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
Information: QA ran without warnings. Pipeline = 27051 |
|
Closing as it breaks the use case of having unit tests compiled in with the runtime capture method. |
5 participants
Tests were previously platform-dependent, as UtilCpuGetNumProcessorsOnline retrieved the real number of CPU cores, and on systems with fewer than 4 CPU cores, some threading unit tests failed.
Link to ticket: https://redmine.openinfosecfoundation.org/issues/7831
Describe changes: