feat: metadata service: make turnserver socket path configurable#840
feat: metadata service: make turnserver socket path configurable#840
Conversation
|
There were no real tests for the turnserver metadata stuff that I could see, so I included that, but I have no real experience with python test framework so that part was vibed but it looks like how I'd do things in other languages. If someone with actual experience in Python tests could review it that would be great |
feld
changed the title
also add tests for the turnserver metadata
feld
force-pushed
the
turnserver-config
branch
from
686edc4 to
0b21b83
Compare
feld
had a problem deploying
to
staging2.testrun.org
GitHub Actions
Error
feld
temporarily deployed
to
staging-ipv4.testrun.org
GitHub Actions
Inactive
|
|
||
| # Allow time for server to start | ||
| import time | ||
| time.sleep(0.01) |
There was a problem hiding this comment.
this doesn't look like a serious way to wait for a service to start, might cause flaky tests 🤔
adbenitez
left a comment
adbenitez
left a comment
There was a problem hiding this comment.
lgtm, I didn't test but the change is simple and straightforward: just adding a new configuration option for a path instead of using a hard-coded value, can't say much about the tests tho better if someone with more deep understanding of the current tests machinery also take a look at it, the use of sleep give me a bad feeling, I didn't check if other tests also do this
There was a problem hiding this comment.
I'm slightly conflicted about this PR.
The additional config option looks innocent enough, it's a bit annoying to add new config options for small things which operators are only expected to edit with alternative deployment options, but fine with me if the chef cookbook needs this. I think my preference is saying "please rebase this on top of #853 so we don't have to put this option into the default-generated chatmail.ini, and can just set a default in config.py for everyone who doesn't need to think about this".
But then there is also the LLM-generated tests; you don't understand what they do, I don't understand what they do, they have some sketchy sleep statement in it... If they were in a separate PR, this one would be long approved.
Also, please fix the lint errors :) https://github.com/chatmail/relay/actions/runs/22113484762/job/63915452488?pr=840
hpk42
left a comment
hpk42
left a comment
There was a problem hiding this comment.
thanks. please simplify the PR along the review comments.
| self.metadata = metadata | ||
| self.iroh_relay = iroh_relay | ||
| self.turn_hostname = turn_hostname | ||
| self.config = config |
There was a problem hiding this comment.
we avoid passing config around in favor of explicit values.
in this case it's the turn_socket_path that should be passed in.
| @@ -0,0 +1,120 @@ | |||
| """Tests for turnserver functionality, particularly metadata integration.""" | |||
There was a problem hiding this comment.
i think it's fine to just do offline config tests (that the config setting exists and has a proper default) and not try to test turn_credentials with mock servers etc. for now. it didn't have these tests before.
There was a problem hiding this comment.
There are now tests for turnserver functionality here: 835b5e7#diff-fc3c6a3f43aaa8266eb10ce6dad7f018493f151fca34cecea33318d6ce719afa
Maybe we can take the turnserver tests out of this PR and use those instead?
4 participants
also add tests for the turnserver metadata
Fixes #779