-
Notifications
You must be signed in to change notification settings - Fork 48
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
fix(test): Enhancing test_http_timeout to include CCT-506 #244
base: master
Are you sure you want to change the base?
Conversation
7a53ca6
to
986053e
Compare
LGTM |
…st-connection not catching ReadTimeoutError Exception
986053e
to
34ec1e0
Compare
I ran the test on my RHEL 9.4 VM and it didn’t fail.
Is it possible that I am doing something wrong? @Archana-PandeyM Did the test fail correctly during your review? |
I think I understood the PR wrong. The test it adds is actually verifying that the ReadTimeoutError is not caught and its traceback is printed. In such case, the test is correct and works as intended. Can you please confirm that, @zpetrace and @Archana-PandeyM? In such case, I think we can undraft, rebase, review and merge this pull request. I am working on a fix, so we can consider CCT-506 accepted. What does @m-horky think? |
I need to update my thoughs once again. I understood the original intention well: this integration test is supposed to fail. assert "Traceback" not in output.stdout ensures that it’s not the case when the exception is properly caught and concisely printed.
You can see there is no traceback, but the error is The actual wrong output this tests verifies that does not get printed is:
In this case, it’s The false positives can be easily avoided by making the error type assert more specific: - assert "timeout=0.01" in output.stdout
+ assert "read timeout=0.01" in output.stdout It however doesn’t resolve the problem of incorrectly testing For the time being, it’s at least possible to distinguish between the two error types by making the asserts more explicit as mentioned above: check the output for |
Enhancing test_http_timeout to include CCT-506 - insights-client --test-connection not catching ReadTimeoutError Exception
So far this PR is a draft as the CCT card is still in refinement