Skip to content

tests/extmod/ussl_basic.py fails for modussl_mbedtls #4364

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

Closed
pfalcon opened this issue Dec 17, 2018 · 6 comments
Closed

tests/extmod/ussl_basic.py fails for modussl_mbedtls #4364

pfalcon opened this issue Dec 17, 2018 · 6 comments
Labels
tests Relates to tests/ directory in source

Comments

@pfalcon
Copy link
Contributor

pfalcon commented Dec 17, 2018

Well, subj. ussl_basic.py is coded too much relying on modussl_axtls behavior.

--- extmod_ussl_basic.py.exp	2018-12-16 22:45:07.539600906 +0300
+++ extmod_ussl_basic.py.out	2018-12-16 22:45:07.539600906 +0300
@@ -1,9 +1,7 @@
-ssl_handshake_status: -256
+mbedtls_ssl_handshake error: -7280
 wrap_socket: OSError(5,)
-<_SSLSocket 
-setblocking: NotImplementedError
-4
-b''
-read: OSError(-261,)
-read: OSError(9,)
-write: OSError(9,)
+mbedtls_ssl_handshake error: -7280
+Traceback (most recent call last):
+  File "extmod/ussl_basic.py", line 18, in <module>
+OSError: [Errno 5] EIO
+CRASH
@Jongy
Copy link
Contributor

Jongy commented Feb 7, 2019

Hmm.. They really expect an inherently different behavior.

What should we do in such case? Is there any option to run tests based on external reading of mpconfig.h? If not, then perhaps we should add a ussl.backend resolving to axtls or mbedtls, then the test run only the relevant cases based on the implementation.

Btw, this partly stems the same problem as I've described in #4475.

@pfalcon
Copy link
Contributor Author

pfalcon commented Feb 7, 2019

That test is just a hack.

then perhaps we should add a ussl.backend

Adding more bytes is not allowed ;-). (Well, in coverage build, everything is allowed).

@Jongy
Copy link
Contributor

Jongy commented Feb 8, 2019

In the CI, run-tests uses the coverage build? If so, I can add a MICROPY_PY_USSL_BACKEND thing, enabled only for coverage.

@pfalcon
Copy link
Contributor Author

pfalcon commented Feb 8, 2019

Well, as I told, that test is hack. Patching it with even more hacks unlikely seems like a good investment of effort.

Instead imagine how a real test would be written: allow to override time and random sources for the underlying TLS lib, then feed predefined/read expected TLS records. Well, could also implement real TLS handling on the tester side ;-).

If it doesn't do that, it's a hack. And if some lines of a hack doesn't work with different TLS implementations, those lines are very bad hack. Their removal is obvious solution. Well, could also keep them in a dirty attempt to keep coverage, just test for variants of expected behavior explicitly (not by printing stuff).

@pfalcon
Copy link
Contributor Author

pfalcon commented Mar 1, 2019

Note that #4481 was posted also as an example that adding any adhoc, mis-leveled tests (which test not TLS, but low-levelish, corner behavior of current implementations) would only complicate matters.

@dpgeorge dpgeorge added the tests Relates to tests/ directory in source label Jul 16, 2022
dpgeorge added a commit to dpgeorge/micropython that referenced this issue Jul 16, 2022
Fixes issue micropython#4364.

Signed-off-by: Damien George <damien@micropython.org>
dpgeorge added a commit to dpgeorge/micropython that referenced this issue Jul 16, 2022
Fixes issue micropython#4364.

Signed-off-by: Damien George <damien@micropython.org>
dpgeorge added a commit to dpgeorge/micropython that referenced this issue Jul 18, 2022
Fixes issue micropython#4364.

Signed-off-by: Damien George <damien@micropython.org>
@dpgeorge
Copy link
Member

Fixed by 18ecc29

karfas pushed a commit to karfas/micropython that referenced this issue Apr 23, 2023
Fixes issue micropython#4364.

Signed-off-by: Damien George <damien@micropython.org>
alphonse82 pushed a commit to alphonse82/micropython-wch-ch32v307 that referenced this issue May 8, 2023
Fixes issue micropython#4364.

Signed-off-by: Damien George <damien@micropython.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Relates to tests/ directory in source
Projects
None yet
Development

No branches or pull requests

3 participants