-
Notifications
You must be signed in to change notification settings - Fork 544
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
[tests] search for fixed strings, not regexps #3320
[tests] search for fixed strings, not regexps #3320
Conversation
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
Particular example where current tests raise false alarms: creating a short-lived VM instance that I set hostname based on its IP address - for IP address, say, 12.34.56.78, I set hostname to ip-12-34-56-78. Then "ip-12-34-56-78" appears in many places in sosreport, and the It detects it wrongly. The hostname is not an IP address but my fault to "obviously hide" IP address in a way that sos is totally unable to detect it as IP address, but a human can interpret it that way. We have three options here:
Still I am in strong favour of the 2nd option / grep fixed string. |
I'm not entirely following what the issue is. 1.2.3.4 is a valid IP address. And ip-12-34-56-78 wouldn't be the ip parser's responsibility to catch, it'd be the hostname parser which does already look for that fixed string. |
Ah right, I simplified my description. The Is it then rather an infrastructure / setting issue that IP address is such visibly "hidden"? Some opinion from @mhradile ? |
If I understand it correctly, we are in situation 1. and need to fix test. Our test API function I do not think it is wise to make this function to take fixed strings. I believe it will find use cases in its previous form (taking regex). I'd vote for adding the new function for fixed strings with a different name+docs and use that new one in those tests instead additionaly fixing current function's doc string. Other option would be to parametrize the original function to take both regex and fixed strings according to user selection. If we absolutely do not need a function taking regex (I think we do for future tests), we should rename the current function to reflect it's doc string and intended purpose. |
Minimalistic reproducer:
which fails on:
since captured
|
For context, the reason we shell out to With that in mind, and the above, I think @mhradile suggestion of parameter-izing this function is the best path forward. I don't have a specific preference for which is the default behavior - but taking a cursory look I think we just happen to have only fixed-string needs right now, so it'd be no objection to me to use |
Ack to that defaults - until I forgotten, I will update the PR. |
Change grep_for_content to grep for a fixed string by default. The reason is tests would match 1a2b3c4 as IP address 1.2.3.4 (which it is not). Add regexp option to grep_for_content, to allow the default "grep" search. Resolves: sosreport#3320 Signed-off-by: Pavel Moravec <pmoravec@redhat.com>
018d3a0
to
7eb0ec9
Compare
grep_for_content should search for a fixed string, not for regexp. The reason is sos is unable to identify 1a2b3c4 as an IP address (which it is not), but grep_for_content would raise false match against 1.2.3.4 .
Resolves: #3320
Please place an 'X' inside each '[]' to confirm you adhere to our Contributor Guidelines