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

Unit tests results should not be displayed in 'automake output' mode #3232

Conversation

eduar-hte
Copy link
Contributor

what

Updated unit_tests program to not show individual unit tests results in automake output.

why

Showing individual results breaks the collection of results when a test fails bypassing its detection, which makes make check show incorrect results.

The issue is that the individual test results include escape characters to colorize the output (to show in red that a test failed for example) and this makes the generated .log & .trs file binary, which breaks grep parsing in test/custom-test-driver and the collection of test results from autotest.

NOTE: The issue is not limited to the escape characters to colorize output as there are tests that include binary data in the input & output of the tests, which when display trigger the same issue.

For example, in the context of #3231, if you introduce a bug in the ParityOdd7bit transformation (by switching the inplace template parameter from true to false, which makes it behave like the ParityEven7bit in the new shared implementation), three of the tests should fail. However, if you run the tests with make check, they complete with zero errors! If you look at the output of the execution, though, you can see that there are grep errors:

(...)
(   4/  0/   4): test/test-cases/secrules-language-tests/transformations/parityEven7bit.json
grep: (standard input): binary file matches
grep: (standard input): binary file matches
grep: (standard input): binary file matches
(   0/  0/   0): test/test-cases/secrules-language-tests/transformations/parityOdd7bit.json
(...)
(   7/  0/   7): test/test-cases/secrules-language-tests/transformations/utf8toUnicode.json
grep: test/test-cases/secrules-language-tests/transformations/parityOdd7bit.json.trs: binary file matches
grep: test/test-cases/secrules-language-tests/transformations/parityOdd7bit.json.trs: binary file matches
grep: test/test-cases/secrules-language-tests/transformations/parityOdd7bit.json.trs: binary file matches
============================================================================
Testsuite summary for modsecurity 3.0
============================================================================
# TOTAL: 4956
# PASS:  4918
# SKIP:  38
# XFAIL: 0
# FAIL:  0
# XPASS: 0
# ERROR: 0
============================================================================

misc

Piggyback on this PR to complete update from PR #3229. 🙏

- Test results output escape characters to highlight whether the test
  passed or failed. Additionally, the input & output for each test can
  include non-ASCII characters. These characters break parsing of
  results (.log & .trs files) with grep, as the files are interpreted
  to be binary.
- Some versions of gcc/libc require setting the pthread flag when using
  std::thread, which to implement it.
- This was found compiling the library in a Debian (bullseye) container.
Copy link

sonarcloud bot commented Aug 18, 2024

@marcstern marcstern added the 3.x Related to ModSecurity version 3.x label Aug 20, 2024
Copy link
Member

@airween airween left a comment

Choose a reason for hiding this comment

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

LGTM

@airween airween merged commit 97c8766 into owasp-modsecurity:v3/master Aug 26, 2024
49 checks passed
@eduar-hte eduar-hte deleted the failed-unit-tests-automake-output branch August 26, 2024 19:48
airween added a commit to airween/ModSecurity that referenced this pull request Aug 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.x Related to ModSecurity version 3.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants