-
Notifications
You must be signed in to change notification settings - Fork 31
Fix failing https.txt test #384
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
Conversation
a2ef35b
to
d62aabe
Compare
After just integrating #387, I've (accidentally/short-sightedly) pressed the "Update branch" button here. I don't know if that was the right decision as it created a merge commit here. Sorry so much if that will create any hassle for you. We might want to rebase onto master and force push here again if that might resolve something. |
src/crate/client/doctests/https.txt
Outdated
@@ -80,5 +80,5 @@ invalid:: | |||
>>> client.server_infos(crate_host) | |||
Traceback (most recent call last): | |||
... | |||
crate.client.exceptions.ConnectionError: Server not available, exception: ...[SSL: ... | |||
crate.client.exceptions.ConnectionError: Server not available, exception:... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there anything else in the traceback that would indicate that the certificate is invalid? Asserting only the ConnectionError
seems a bit generic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On this matter, I just received this error with traceback (from [1]):
File "D:\a\crate-python\crate-python\src\crate\client\doctests\https.txt", line 80, in https.txt
Failed example:
client.server_infos(crate_host)
Expected:
Traceback (most recent call last):
...
crate.client.exceptions.ConnectionError: Server not available, exception: ...[SSL: ...
Got:
Traceback (most recent call last):
File "C:\hostedtoolcache\windows\Python\3.7.9\x64\lib\doctest.py", line 1337, in __run
compileflags, 1), test.globs)
File "<doctest https.txt[11]>", line 1, in <module>
client.server_infos(crate_host)
File "D:\a\crate-python\crate-python\src\crate\client\http.py", line 402, in server_infos
response = self._request('GET', '/', server=server)
File "D:\a\crate-python\crate-python\src\crate\client\http.py", line 502, in _request
"Server not available, exception: %s" % ex_message
crate.client.exceptions.ConnectionError: Server not available, exception: HTTPSConnectionPool(host='localhost', port=65534): Max retries exceeded with url: / (Caused by ProtocolError('Connection aborted.', ConnectionResetError(10054, 'An existing connection was forcibly closed by the remote host', None, 10054, None)))
The particular exception is
ConnectionResetError(10054, 'An existing connection was forcibly closed by the remote host', None, 10054, None)
so there is nothing in there which might indicate the certificate is invalid, unless this error got masked by any other phenomenon.
[1] https://github.com/crate/crate-python/pull/388/checks?check_run_id=1348073153#step:5:156
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unless this error got masked by any other phenomenon
Please forget about the gist of my last comment. The reason for the ConnectionResetError
is coming from the fact that CrateDB has not spun up completely, see also #388 (comment).
However, maybe that is also the reason for the flakyness within other test suite invocations? If so, increasing wait time from 30 seconds to 45 seconds might also help, as I've found out during my work on #388.
crate-python/src/crate/testing/layer.py
Line 348 in 912acda
if wait_time > 30: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is, this failing https test doesnt actually set up a cratedb cluster to do it. It just creates a generic https server and attempts to query it with the client, see: https://github.com/crate/crate-python/blob/master/src/crate/client/tests.py#L210
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Thanks!
While I haven't been able to spot this error when running the test suite on my machine, I received respective hiccups on CI and reported about them at #386 (comment). @autophagy: Can you look at the comment by @chaudum and then let us aim at getting this merged? |
d30be0d
to
f646644
Compare
Ha. Was it really just about adding
I would have expected to improve on the flakeyness then by either giving it some more time to spin up at crate-python/src/crate/client/tests.py Line 251 in 1f3189e
or otherwise wait for the socket being actually responsive like [1] (just to get the idea, here it is HTTP-based) or similar to [2,3] (maybe better because it is TCP-based already)? [1] crate-python/src/crate/testing/layer.py Lines 360 to 371 in 1f3189e
[2] https://github.com/lovelysystems/lovely.testlayers/blob/0.7.0/src/lovely/testlayers/util.py#L6-L13 [3] https://github.com/lovelysystems/lovely.testlayers/blob/0.7.0/src/lovely/testlayers/server.py#L77-L88 |
We can merge this anyway and see if the issue will come back again. Thanks! |
This error occurs in some python version tests but not in others - I believe the actual exception message differs across different library versions, but the outcome is ultimately the same.