Use HTTPStatus constants in place of literals in tests

[dklimpel]

Pull Request Checklist

• Pull request is based on the develop branch
• Pull request includes a changelog file. The entry should:
  • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
  • Use markdown where necessary, mostly for code blocks.
  • End with either a period (.) or an exclamation mark (!).
  • Start with a capital letter.
  • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
• Pull request includes a sign off
• Code style is correct
  (run the linters)

Signed-off-by: Dirk Klimpel dirk@klimpel.org

[richvdh]

I'm not convinced this is an improvement. For tests, I'd rather we were very explicit about the response code.

(Note that HTTPStatus.* are magic objects rather than simple strings or ints, so comparing them with a string is not a completely trivial operation.)

[dklimpel]

That's ok. I don't prefer any particular direction. But at the moment the tests are different. Perhaps there is a lack of clear guidelines or documentation.

[richvdh]

@matrix-org/synapse-core: what does everyone else think about this? I'd rather we used explicit values in test code, and considered use of HTTPStatus.* values in tests a bug.

[DMRobertson]

@matrix-org/synapse-core: what does everyone else think about this? I'd rather we used explicit values in test code, and considered use of HTTPStatus.* values in tests a bug.

This one is my fault. I prefer the enum values because I can never remember what the 3xx or 2xx status codes (other than 200) mean. But I don't have a particularly strong opinion.

Edit:

Note that HTTPStatus.* are magic objects rather than simple strings or ints, so comparing them with a string is not a completely trivial operation.

I'm not sure what you mean here; I thought we parse HTTP status codes as decimal integers and expect them to be an integer within Synapse? If so, I don't see what the problem is with comparisons:

>>> from http import HTTPStatus
>>> HTTPStatus.OK == 200
True
>>> HTTPStatus.OK == "200"
False
>>> HTTPStatus.OK == b"200"
False

[richvdh]

This one is my fault. I prefer the enum values because I can never remember what the 3xx or 2xx status codes (other than 200) mean.

This is kinda my point. The spec tends to give numerical values rather than FORBIDDEN or UNAUTHORIZED, and using the enum values means I have to go and look them up to see if we're testing for a 401 or a 403.

Note that HTTPStatus.* are magic objects rather than simple strings or ints, so comparing them with a string is not a completely trivial operation.

I'm not sure what you mean here; I thought we parse HTTP status codes as decimal integers and expect them to be an integer within Synapse?

In this case we're in test code rather than "within Synapse" per se, and the thing we're trying to check is the value that was passed to FakeChannel.writeHeaders, which is a bytes rather than an int.

Mostly I mean I'm twice-shy about considering HTTPStatus to be interchangeable with an int after #7017 and #11812, so I want to make sure we're actually sending the bytes 200 rather than the bytes HTTPStatus.OK.

In other words: I want test code to be as explicit as possible.

(On closer inspection, FakeChannel.code() includes an int(...) cast, so we'd probably catch an attempt to send something stupid anyway...)

[erikjohnston]

I don't have a particularly strong opinion either way TBH.

[richvdh]

Right, given:

• I feel fairly strongly this is wrong
• I think I've persuaded @DMRobertson
• Nobody else seems to care, provided we're consistent

... I'm making an executive decision to reject this PR, with our thanks.

@dklimpel if you want to make a PR changing things in the other direction, it would be welcome! 😇