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/freshclam_test.py: fix unit test #881

Merged
merged 1 commit into from Apr 21, 2023

Conversation

arjendekorte
Copy link
Contributor

Somehow the tests returns a different result than expected. Not sure if the expected result should be changed (this patch) or that the message emitted by freshclam should be changed.

Somehow the tests returns a different result than expected. Not sure if the expected result should be changed (this patch) or that the message emitted by freshclam should be changed.
@arjendekorte
Copy link
Contributor Author

String to match is generated in libfreshclam/libfreshclam_internal.c line 2144

@opoplawski
Copy link
Contributor

This certainly does resolve the test failure. Still to be determined I suppose if this is the correct fix.

@arjendekorte
Copy link
Contributor Author

Indeed. Changing line 2144 in libfreshclam/libfreshclam_internal.c will have the same result, but I have no idea either whether or not this is intentionally different.

@micahsnyder
Copy link
Contributor

After thinking about this for some time, I suspect you are carrying a patch that makes Debug-level messages go to stderr. This test is checking for a Debug-level message in stdout. Can you confirm?

@opoplawski's proposed change switches to look for an Info-level message in stdout that is roughly equivalent. I'm also happy with this change.

@micahsnyder
Copy link
Contributor

Before I merge, if you can confirm the debug/stderr suspicion that may allow us to fix up the commit message.

@opoplawski
Copy link
Contributor

I don't believe I have any patches that are changing where any messages might be going.

@micahsnyder
Copy link
Contributor

I don't believe I have any patches that are changing where any messages might be going.

Thanks for checking. I'm very confused how the test is failing for you without this change. I have never seen that test fail, and just tried reproducing it locally on fedora:latest. Perhaps it is something in your build environment, then? I don't know.

In the interests of understanding why it is failing for you, I am very curious if this change would cause the test to pass for you as well, instead of the change in this PR:

diff --git a/libfreshclam/libfreshclam.c b/libfreshclam/libfreshclam.c
index eda780374..33a9c0e45 100644
--- a/libfreshclam/libfreshclam.c
+++ b/libfreshclam/libfreshclam.c
@@ -640,7 +640,7 @@ fc_error_t fc_update_database(
                     if (*bUpdated) {
                         logg(LOGG_DEBUG, "fc_update_database: %s updated.\n", dbFilename);
                     } else {
-                        logg(LOGG_DEBUG, "fc_update_database: %s already up-to-date.\n", dbFilename);
+                        logg(LOGG_INFO, "fc_update_database: %s already up-to-date.\n", dbFilename);
                     }
                     goto success;
                 }

Can you please test it? It's not a good change, but would prove or disprove my thoughts about the LOG_DEBUG messages going to stderr instead of stdout.

@opoplawski
Copy link
Contributor

That patch fixes the test for me as well. FWIW - my ctest line is:

+ /usr/bin/ctest --test-dir redhat-linux-build --output-on-failure --force-new-ctest-process -j4 -- -E valgrind

@micahsnyder micahsnyder merged commit 629c018 into Cisco-Talos:main Apr 21, 2023
16 of 24 checks passed
@micahsnyder
Copy link
Contributor

I merged the commit and updated the commit message to explain the reason for the change and the outstanding issue. I am still curious to solve why debug-level messages aren't bring printed for you.

@arjendekorte arjendekorte deleted the patch-2 branch April 23, 2023 01:43
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.

None yet

3 participants