Conversation
alexwlchan
left a comment
alexwlchan
left a comment
There was a problem hiding this comment.
One question about the test, but the changelog/code look fine.
| # agree that these settings have been ACK'd, so their underlying | ||
| # dictionaries should be identical. | ||
| assert ( | ||
| client.local_settings._settings == server.remote_settings._settings |
There was a problem hiding this comment.
This tells us that after we’ve exchanged some frames, the server/client settings match – but do we know they weren’t the same to start with? Should there be a second assert at the start of the test:
assert (
client.local_settings._settings != server.remote_settings._settings
)so we know sending the settings to the server really did have an effect on its settings?
There was a problem hiding this comment.
I’m slightly annoyed that if the new precondition fails, we’ll get a failure (bug in product code) rather than an error (bug in test setup).
But AFAIK, the only way pytest can throw an error is inside a fixture, and it’s not worth breaking this into a separate fixture for what’s a pretty trivial distinction.
There was a problem hiding this comment.
I think there is an alternative, which is to define __eq__ for the settings class. There is probably some value in that, but I shied away from it because I was concerned about adding all those extra tests in this PR.
alexwlchan
left a comment
alexwlchan
left a comment
There was a problem hiding this comment.
That issue resolved, LGTM. 👍
| # agree that these settings have been ACK'd, so their underlying | ||
| # dictionaries should be identical. | ||
| assert ( | ||
| client.local_settings._settings == server.remote_settings._settings |
There was a problem hiding this comment.
I’m slightly annoyed that if the new precondition fails, we’ll get a failure (bug in product code) rather than an error (bug in test setup).
But AFAIK, the only way pytest can throw an error is inside a fixture, and it’s not worth breaking this into a separate fixture for what’s a pretty trivial distinction.
2 participants
Resolves #451.