-
Notifications
You must be signed in to change notification settings - Fork 721
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
Convert homespun tests to conventional tests #780
Conversation
e889845
to
e7b0a34
Compare
pytest, not py.test since version 3.0 |
5885aba
to
bf83885
Compare
old naming dies hard 😂 |
bf83885
to
d54cea3
Compare
Ah, I'll improve this a little more – we'll want to be sure the client process didn't die with an exception and so on... Also turns out the original author(s) of the clients have been funny with
where they actually mean
|
63f1758
to
e381f43
Compare
e381f43
to
7720250
Compare
7720250
to
2413572
Compare
Phew, this PR underwent a bit of a metamorphosis since last night, but I think it's much, much better now. We will now be able to automagically catch exceptions from the clients, and there's (almost) no playing with return codes and so on. I also fixed the way port A bunch of actually unused/no-op code also got removed – a 723 line removal is pretty good in my books 😁 I think there's still more unused code, but the MQTTv5 tests would need to be fixed first (#769) to see if that's the case; they don't actually run against the current |
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.
Great works ! That huge works, thank for your time.
@@ -25,13 +24,26 @@ class TestError(Exception): | |||
def __init__(self, message="Mismatched packets"): | |||
self.message = message | |||
|
|||
|
|||
def bind_to_any_free_port(sock) -> int: |
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 will no longer have strange error when I run a real broker on port 1888 and run paho test.
PYTHONPATH=f"{tests_path}{os.pathsep}{os.environ.get('PYTHONPATH', '')}", | ||
) | ||
assert 'PAHO_SERVER_PORT' in env, "PAHO_SERVER_PORT must be set in the environment when starting a client" | ||
# TODO: it would be nice to run this under `coverage` too! |
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.
We might be able to have coverage done it instead of a subprocess we start a thread. But this is probably easy to said and harder to implement. Anyway I think it not in the scope of this PR.
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.
pytest-cov
actually hooks things up so subprocesses work OOTB, which was nice to notice!
No problem, it was a nice brain workout while watching Christmas movies and drinking mulled wine and cognac... 😂 |
This PR converts the homespun
test/
code to (what I think is) pretty idiomatic Python/Pytest code.I think I got everything right, the original code was pretty hairy. 😂
Would probably be even better if the client code was included in the tests themselves to avoid having to hop between files.
Fixes #768