-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Add support to run unit tests in a multithreaded context #3221
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
Add support to run unit tests in a multithreaded context #3221
Conversation
All operator & transformation unit tests were executed using this feature and no issue was detected.
NOTE: This doesn't guarantee that there's no multithreading issue lurking in there, though.
7a6052a to
b67bc19
Compare
- This is controlled by specifying the 'mtstress' argument when running `unit_test`. - The goal is to detect if the operator/transformation fails in this context. - In this mode, the test will be executed 5'000 times in 50 threads concurrently. - Allocation & initialization of the operator/transformation is performed once in the main thread, while the evaluation is executed in the threads. - This is consistent with the library's support for multithreading, where initialization and loading of rules is expected to run once. See issue owasp-modsecurity#3215.
b67bc19 to
ee5f95e
Compare
Quality Gate Passed Quality Gate passed
Issues
2 New issues
0 Accepted issues
Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code
c9af0c7
into
owasp-modsecurity:v3/master
Thanks, @eduar-hte!
A reminder to me: 11 tests are failed, I have to check the reason.
A reminder to me: 11 tests are failed, I have to check the reason.
I think you're referring to these unit tests: test-cases/secrules-language-tests/transformations/phpArgsNames.json.
I remember looking into this while working on the Windows port (PR #3132), because when I run unit_tests manually they'd fail (they're not part of the make check test suite (test/test-suite.in) and that's why the builds don't fail.)
These tests were added to in owasp-modsecurity/secrules-language-tests@a3d4405 and are related to PR #2387, which I understand is a v3.1 feature.
I think you're referring to these unit tests:
test-cases/secrules-language-tests/transformations/phpArgsNames.json.
yes, exactly.
I remember looking into this while working on the Windows port (PR #3132), because when I run
unit_testsmanually they'd fail (they're not part of themake checktest suite (test/test-suite.in) and that's why the builds don't fail.)
yes, I also wondered why this haven't appeared yet
These tests were added to in owasp-modsecurity/secrules-language-tests@a3d4405 and are related to PR #2387, which I understand is a v3.1 feature.
ah, thanks - I hadn't time to review the history, but yes, this explains the behavior. Seems like the tests had already added but the transformation not. I think we can remove this test file.
Thanks for helping discovery the situation.
ah, thanks - I hadn't time to review the history, but yes, this explains the behavior. Seems like the tests had already added but the transformation not. I think we can remove this test file.
If you don't want to remove it, you could update the submodule reference in test/test-cases/secrules-language-tests to reference the previous commit (owasp-modsecurity/secrules-language-tests@d03f4c1), as the commit with the phpArgsNames tests is the latest commit in that repository.
ah, thanks - I hadn't time to review the history, but yes, this explains the behavior. Seems like the tests had already added but the transformation not. I think we can remove this test file.
Submitted PR owasp-modsecurity/secrules-language-tests#10 to remove the tests associated to this missing transformation.
Uh oh!
There was an error while loading. Please reload this page.
what
The
unit_testprogram has been updated to accept themtstressargument to run operator/transformation tests in a multithreaded context.why
The goal is to detect if the operator/transformation fails in this context.
misc
references
This work is related to #3215, to check whether the optional mutex on the Pm operator to access
acmptrees is needed.This was introduced in commit 119a6fc & 7d786b3 likely because of issue #1573. This option is off by default and it's not clear whether it's necessary, as stated here.