Fix tests on Twisted trunk. #16528

clokep · 2023-10-20T13:42:38Z

Part of #16289, before fixing them we need to refactor some code first.

Twisted trunk makes a change to the TLSMemoryBIOFactory where the underlying protocol is changed from TLSMemoryBIOProtocol to BufferingTLSTransport to improve performance of TLS code (see twisted/twisted#11989). In order to properly hook up this code we need to modify some of our test code to pass the test reactor's clock into TLSMemoryBIOFactory such that the global (trial) reactor isn't used by default.

This is all rather annoying, especially when attempting to maintain compatible with older Twisted versions.

See https://github.com/twisted/twisted/pull/11996/files#diff-9d27cdf01b21540c6ef350193a9d44b88a3d2b322f3fbc08b95cdaac8fd50ed6R871-R874 for where Twisted does this in its own tests.

DMRobertson · 2023-10-23T13:10:52Z

tests/http/__init__.py

+    # Twisted > 23.8.0 has a different API that accepts a clock.
+    if twisted.version <= Version("Twisted", 23, 8, 0):
+        return TLSMemoryBIOFactory(
+            connection_creator, isClient=False, wrappedFactory=factory
+        )
+    else:
+        return TLSMemoryBIOFactory(
+            connection_creator, isClient=False, wrappedFactory=factory, clock=clock  # type: ignore[call-arg]
+        )


Did Twisted make a breaking change to TLSMemoryBIOFactory here, or are we just taking advantage of a newer API?

(I can't see anything in https://github.com/twisted/twisted/blob/trunk/NEWS.rst#deprecations-and-removals so assuming we're using a newer API.)

It isn't a breaking change, you don't have to provide clock, but you do if you want our tests to pass. (Otherwise it uses the trial reactor instead of the test reactor.)

tests/server.py

DMRobertson

Looks sane, but I don't understand what exactly was failing and why on Twisted trunk. Could you spell it out for me?

DMRobertson · 2023-10-23T13:23:50Z

Looks sane, but I don't understand what exactly was failing and why on Twisted trunk. Could you spell it out for me?

Oh, I think I see: twisted/twisted#11996 made speedups in twisted. But as a consequence of how they did so, their tests now need to pass in a Clock explicitly. We need to do the same.

clokep · 2023-10-23T13:27:36Z

Looks sane, but I don't understand what exactly was failing and why on Twisted trunk. Could you spell it out for me?

I attempted to add more info to the description, I'm not sure it is enough though.

DMRobertson · 2023-10-23T13:29:11Z

Thanks! I think I was confused because the version check we added made me think there was a behaviour change in 23.8.0, rather than in twisted trunk.

DMRobertson

Thanks!

clokep · 2023-10-23T13:32:53Z

Thanks! I think I was confused because the version check we added made me think there was a behaviour change in 23.8.0, rather than in twisted trunk.

There's no released version so I had to do checks against the currently released version (and hope there's no 23.8.x release.... but I find that unlikely).

…-tests

clokep · 2023-10-23T20:15:05Z

Looks like the patching is causing issues... maybe a memory leak?

This reverts commit 5dcaea6.

clokep · 2023-10-24T11:32:00Z

I kicked off a run without the patching: https://github.com/matrix-org/synapse/actions/runs/6626203043

clokep · 2023-10-24T11:49:20Z

I kicked off a run without the patching: matrix-org/synapse/actions/runs/6626203043

Looks like the tests do fail without this change, I wonder if they pass on the commit before?

clokep · 2023-10-24T12:43:00Z

https://github.com/matrix-org/synapse/actions/runs/6626962438

clokep · 2023-10-24T13:37:35Z

matrix-org/synapse/actions/runs/6626962438

This passed, but fails on older Twisteds, I think we should just not patch older ones then? 🤷

clokep · 2023-10-24T14:03:11Z

tests/server.py

+        # test to patch over it.
+        #
+        # Use create=True for backwards compatibilty with Twisted <= 23.8.0.
+        if twisted.version > Version("Twisted", 23, 8, 0):


It feels janky that we check > here and <= to above... as if there is at 23.8.x then it'll enable one bit of code and not the other.

It sounds like a Twisted 23.1x.0 will be out soon-ish though.

23.10, according to https://mail.python.org/archives/list/[email protected]/thread/B7FQO3BIAGPFLQMAOALUZQ2EX3AQDPT6/#4FADHHQ5H4U4BZ6LGWI5PV5K7MBOGK2V

I can use >= 23.10.0 if we want (in both places), but it'll fail until the release is out.

I think 23.8 is fine, just a note for completeness.

clokep · 2023-10-24T14:54:07Z

Tests passed here, now let's see if they pass on Twisted Trunk: https://github.com/matrix-org/synapse/actions/runs/6628632702

clokep · 2023-10-24T16:56:53Z

Let's try again: https://github.com/matrix-org/synapse/actions/runs/6630078646

clokep · 2023-10-24T18:03:57Z

@DMRobertson Can you take another quick look?

DMRobertson · 2023-10-25T10:45:48Z

tests/http/test_proxyagent.py

-        server_ssl_protocol = _wrap_server_factory_for_tls(
-            _get_test_protocol_factory()
+        server_ssl_protocol = wrap_server_factory_for_tls(
+            _get_test_protocol_factory(), self.reactor, sanlist=[b"DNS:test.com"]


Is passing in an explicit sanlist here a meaningful change?

Ditto below in the rest of this test

No: _wrap_server_factory_for_tls defaulted to this if no sanlist argument was provided.

Some tests are known to fail with >twisted-23.8, see [1] [1] matrix-org/synapse#16528 Signed-off-by: Petr Vaněk <[email protected]>

Some tests are known to fail with >twisted-23.8, see [1] [1] matrix-org/synapse#16528 Signed-off-by: Petr Vaněk <[email protected]> Signed-off-by: Sam James <[email protected]>

clokep added 3 commits October 20, 2023 09:16

Combine duplicated code.

05282b3

Fix tests for Twisted > 23.8.0.

c6afdde

Newsfragment

f4efd04

DMRobertson reviewed Oct 23, 2023

View reviewed changes

tests/server.py Outdated Show resolved Hide resolved

DMRobertson reviewed Oct 23, 2023

View reviewed changes

Clarify comment.

69d0181

DMRobertson approved these changes Oct 23, 2023

View reviewed changes

clokep added 2 commits October 23, 2023 14:31

Merge remote-tracking branch 'origin/develop' into clokep/fix-twisted…

15107e6

…-tests

TEMP: Test w/o patch.

5dcaea6

Revert "TEMP: Test w/o patch."

79c76e6

This reverts commit 5dcaea6.

Fix tests?

3467806

clokep commented Oct 24, 2023

View reviewed changes

clokep added 2 commits October 24, 2023 12:47

Another try.

01d1e83

Lint

6dec5c3

Lint

412d593

clokep marked this pull request as ready for review October 24, 2023 18:03

clokep requested a review from a team as a code owner October 24, 2023 18:03

clokep requested a review from DMRobertson October 24, 2023 18:03

clokep removed the request for review from a team October 24, 2023 18:03

DMRobertson reviewed Oct 25, 2023

View reviewed changes

clokep merged commit e182dbb into develop Oct 25, 2023
35 checks passed

clokep deleted the clokep/fix-twisted-tests branch October 25, 2023 11:39

arkamar added a commit to arkamar/gentoo that referenced this pull request Nov 1, 2023

net-im/synapse: restrict to <=dev-python/twisted-23.8.0

c361bed

Some tests are known to fail with >twisted-23.8, see [1] [1] matrix-org/synapse#16528 Signed-off-by: Petr Vaněk <[email protected]>

arkamar added a commit to arkamar/gentoo that referenced this pull request Nov 1, 2023

net-im/synapse: restrict to <=dev-python/twisted-23.8.0

5bca688

Some tests are known to fail with >twisted-23.8, see [1] [1] matrix-org/synapse#16528 Signed-off-by: Petr Vaněk <[email protected]>

arkamar added a commit to arkamar/gentoo that referenced this pull request Nov 1, 2023

net-im/synapse: restrict <dev-python/twisted-23.8.1

f9fc83d

Some tests are known to fail with >twisted-23.8, see [1] [1] matrix-org/synapse#16528 Signed-off-by: Petr Vaněk <[email protected]>

arkamar added a commit to arkamar/gentoo that referenced this pull request Nov 17, 2023

net-im/synapse: restrict <dev-python/twisted-23.8.1

1dab624

Some tests are known to fail with >twisted-23.8, see [1] [1] matrix-org/synapse#16528 Signed-off-by: Petr Vaněk <[email protected]>

devonh mentioned this pull request May 30, 2024

Allow twisted versions >=23.10 matrix-org/sygnal#381

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix tests on Twisted trunk. #16528

Fix tests on Twisted trunk. #16528

clokep commented Oct 20, 2023 •

edited

Loading

DMRobertson Oct 23, 2023

DMRobertson Oct 23, 2023

clokep Oct 23, 2023 •

edited

Loading

DMRobertson left a comment

DMRobertson commented Oct 23, 2023

clokep commented Oct 23, 2023

DMRobertson commented Oct 23, 2023

DMRobertson left a comment

clokep commented Oct 23, 2023

clokep commented Oct 23, 2023 •

edited

Loading

clokep commented Oct 24, 2023

clokep commented Oct 24, 2023

clokep commented Oct 24, 2023

clokep commented Oct 24, 2023

clokep Oct 24, 2023

DMRobertson Oct 25, 2023

clokep Oct 25, 2023

DMRobertson Oct 25, 2023

clokep commented Oct 24, 2023

clokep commented Oct 24, 2023

clokep commented Oct 24, 2023

DMRobertson Oct 25, 2023

DMRobertson Oct 25, 2023

clokep Oct 25, 2023

Fix tests on Twisted trunk. #16528

Fix tests on Twisted trunk. #16528

Conversation

clokep commented Oct 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clokep Oct 23, 2023 • edited Loading

Choose a reason for hiding this comment

DMRobertson left a comment

Choose a reason for hiding this comment

DMRobertson commented Oct 23, 2023

clokep commented Oct 23, 2023

DMRobertson commented Oct 23, 2023

DMRobertson left a comment

Choose a reason for hiding this comment

clokep commented Oct 23, 2023

clokep commented Oct 23, 2023 • edited Loading

clokep commented Oct 24, 2023

clokep commented Oct 24, 2023

clokep commented Oct 24, 2023

clokep commented Oct 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clokep commented Oct 24, 2023

clokep commented Oct 24, 2023

clokep commented Oct 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clokep commented Oct 20, 2023 •

edited

Loading

clokep Oct 23, 2023 •

edited

Loading

clokep commented Oct 23, 2023 •

edited

Loading