Resolve issue with plaintext upgrade on the server side

[Lukasa]

Resolves #451.

[alexwlchan]

One question about the test, but the changelog/code look fine.

[alexwlchan]

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?

[Lukasa]

Yeah, good call.

[Lukasa]

Done. =D

[alexwlchan]

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.

[Lukasa]

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]

That issue resolved, LGTM. 👍

[alexwlchan]

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.