#11812 Mark TerminalRealm as implementing IRealm #11813

DMRobertson · 2023-02-25T00:00:48Z

Scope and purpose

Fixes #11812.
Related to matrix-org/synapse#15097.

Contributor Checklist:

This process applies to all pull requests - no matter how small.
Have a look at our developer documentation before submitting your Pull Request.

Below is a non-exhaustive list (as a reminder):

The title of the PR should describe the changes and starts with the associated issue number, like “#9782 Remove twisted.news. #1234 Brief description”.
A release notes news fragment file was create in src/twisted/newsfragments/ (see: Release notes fragments docs.)
The automated tests were updated.
Once all checks are green, request a review by leaving a comment that contains exactly the string please review.
Our bot will trigger the review process, by applying the pending review label
and requesting a review from the Twisted dev team.

DMRobertson · 2023-02-25T00:26:36Z

AFAICS the test failure isn't related. A flake?

DMRobertson · 2023-02-25T00:28:27Z

please review

glyph · 2023-02-25T11:05:00Z

Probably a flake. I've re-run the failed job.

adiroiban

Many thanks for the changes.

I think that we need a test for this change.

I have this helper in my tests to check for interface implementations.

    def assertProvides(self, interface, obj):
        self.assertTrue(
            interface.providedBy(obj),
            'Object %s does not provided interface %s.' % (obj, interface))
        from zope.interface.verify import verifyObject
        verifyObject(interface, obj)

I think that something like this can be used to check

sut = TerminalRealm()
self.assertProvides(IRealm, sut)

I see the tests for this files are declared as follows, but I don't see any existing test inside src/twisted/conch/test/test_manhole.py for this class.

# -*- test-case-name: twisted.conch.test.test_manhole -*-

but I think a simple instantiation should be enough.

Thanks again!

exarkun · 2023-03-02T16:45:47Z

Many thanks for the changes.

I think that we need a test for this change.

I have this helper in my tests to check for interface implementations.
    def assertProvides(self, interface, obj):
        self.assertTrue(
            interface.providedBy(obj),
            'Object %s does not provided interface %s.' % (obj, interface))
        from zope.interface.verify import verifyObject
        verifyObject(interface, obj)
I think that something like this can be used to check
sut = TerminalRealm()
self.assertProvides(IRealm, sut)

If such a test is added, I suggest doing it with hamcrest instead of a new TestCase method, so that the assertion looks like:

assert_that(sut, provides(IRealm))

making it much easier to re-use this infrastructure across different tests.

DMRobertson · 2023-03-09T15:28:45Z

I have added a test and a new hamcrest matcher.

If you'll allow me to grumble: while I appreciate that .providedBy and .verifyObject are runtime-visible behaviour, it seems like a lot of overhead for a one-line, drive-by type hint fix.

please review

adiroiban · 2023-03-09T16:25:29Z

Thanks for the changes. Much appreciated.

If you'll allow me to grumble: while I appreciate that .providedBy and .verifyObject are runtime-visible behaviour, it seems like a lot of overhead for a one-line, drive-by type hint fix.

I understand your complaint.
My understanding is that the current quality assurance policy for the Twisted project is to make sure any change is covered by automated tests.
This includes changes for code that was not previously covered.

If you think that my understanding of the current policy is wrong, or you think that it should be changed, I think that we can discuss this over the mailing list or IRC/Gitter ...(but gitter migration is kind of a mess right now, so better email)

DMRobertson · 2023-03-10T11:22:49Z

I don't know why the latest test run had a job that timed out, but the new test was skipped as intended, so I would assume it is not a regression.

adiroiban

Sorry for the late review.

The interface changes are ok.

I am not very happy with the tests :) and the missing test coverage

If you think that hamcrest is better, we can have this merged.

for me self.assertProvides(NonemptyInterface, MissingDecorator()) and self.assertNotProvides() is easier to read and write than

 assert_that(
            provides(NonemptyInterface).matches(MissingDecorator()), equal_to(False)
        )

adiroiban · 2023-10-12T11:32:11Z

src/twisted/conch/test/test_manhole_ssh.py

+    def test_TerminalRealm_implements_IRealm(self) -> None:
+        """Regression test for #11812."""
+        assert_that(TerminalRealm(), provides(IRealm))


I don't have a much better suggestion,

but instead of pointing to a bug ID is best to describe the scope and purpose of the test.

Suggested change

def test_TerminalRealm_implements_IRealm(self) -> None:

"""Regression test for #11812."""

assert_that(TerminalRealm(), provides(IRealm))

def test_interfaces(self) -> None:

"""

Generic check that the interfaces are implemented.

"""

assert_that(TerminalRealm(), provides(IRealm))

adiroiban · 2023-10-12T11:34:10Z

src/twisted/conch/test/test_manhole_ssh.py

+except ImportError:
+    ssh = False
+else:
+    ssh = True
+
+
+class TerminalRealmTests(unittest.TestCase):
+    if not ssh:
+        skip = "cannot import t.conch.manhole_ssh (cryptography requirements missing?)"


minor comment.

mabye we can remove the last if :)

Suggested change

except ImportError:

ssh = False

else:

ssh = True

class TerminalRealmTests(unittest.TestCase):

if not ssh:

skip = "cannot import t.conch.manhole_ssh (cryptography requirements missing?)"

except ImportError:

skipNoConch = 'No t.conch.manhole_ssh available.

else:

skipNoConch = None

class TerminalRealmTests(unittest.TestCase):

skip = skipNoConch

adiroiban · 2023-10-12T11:35:20Z

src/twisted/trial/test/matchers.py

+        return True
+
+    def describe_mismatch(self, item: object, description: Description) -> None:
+        if self._verify_exception is None:


I think that we are missing some tests for invalid interface implementations.

adiroiban · 2023-10-12T11:42:21Z

src/twisted/trial/test/test_matchers.py

+        class EmptyClass:
+            ...
+
+        assert_that(provides(EmptyInterface).matches(EmptyClass()), equal_to(True))


I am not familiar with matchers.
But I fell the final equals_to(True) or equals_to(False is verbose

I was expecting something like

- assert_that(provides(EmptyInterface).matches(EmptyClass()), equal_to(True)) + assert_that(providers(EmptyInterface).matches(EmptyClass())) + assert_that(not().providers(EmptyInterface).matches(EmptyClass()))

adiroiban · 2023-10-12T11:46:12Z

src/twisted/trial/test/matchers.py

@@ -93,3 +97,46 @@ def getContent(p: IFilePath) -> str:
            return f.read().decode(encoding)

    return after(getContent, m)
+
+
+class _MatchInterfaceProvider(BaseMatcher[object]):


I don't know what to say about these matchers :)

The assertion helper is needed.

I have something similar using old-school xunit assertion style :)

def assertProvides(self, interface, obj): self.assertTrue( interface.providedBy(obj), 'Object %s does not provided interface %s.' % (obj, interface)) verifyObject(interface, obj) def assertNotProvides(self, interface, obj): self.assertFalse( interface.providedBy(obj), 'Object %s does not provided interface %s.' % (obj, interface))

https://github.com/chevah/compat/blob/master/src/chevah_compat/testing/assertion.py#L219-L228

Maybe, I am just getting old, but I find these hamcrest part too complicated :)

I know that old-style xunit is based on inheritance , but we are already writing tests in xunit style

DMRobertson · 2023-11-03T15:00:20Z

Thanks for your response. I don't think I have the time to see this through at the moment, so I will close this PR. With that said, I'd be more than happy for anyone else to take this over.

glyph · 2023-11-03T20:07:53Z

Thanks for your response. I don't think I have the time to see this through at the moment, so I will close this PR. With that said, I'd be more than happy for anyone else to take this over.

Thanks for taking the time to contribute, and thanks for letting us know - always better to have a clear response than a PR left in limbo forever!

DMRobertson added 2 commits February 24, 2023 23:57

Mark TerminalRealm as implementing IRealm

cc80620

Changelog

541c7b6

chevah-robot added the needs-review label Feb 25, 2023

chevah-robot requested a review from a team February 25, 2023 00:26

glyph marked this pull request as ready for review February 25, 2023 11:04

Merge branch 'trunk' into dmr/terminal-realm-implements-irealm

0457ff1

adiroiban requested changes Mar 2, 2023

View reviewed changes

chevah-robot added needs-changes and removed needs-review labels Mar 2, 2023

DMRobertson added 2 commits March 9, 2023 15:26

Define new provides matcher

ec0df18

Explictly check TerminalRealm implements IRealm

ff07c6e

chevah-robot added needs-review and removed needs-changes labels Mar 9, 2023

chevah-robot requested a review from a team March 9, 2023 15:36

Skip test if cryptography is not available

5b913da

DMRobertson mentioned this pull request Apr 26, 2023

CI run against Twisted trunk is failing matrix-org/synapse#15097

Closed

Merge branch 'trunk' into dmr/terminal-realm-implements-irealm

cac391c

adiroiban requested changes Oct 12, 2023

View reviewed changes

chevah-robot added needs-changes and removed needs-review labels Oct 12, 2023

DMRobertson closed this Nov 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#11812 Mark TerminalRealm as implementing IRealm #11813

#11812 Mark TerminalRealm as implementing IRealm #11813

DMRobertson commented Feb 25, 2023 •

edited

DMRobertson commented Feb 25, 2023 •

edited

DMRobertson commented Feb 25, 2023

glyph commented Feb 25, 2023

adiroiban left a comment •

edited

exarkun commented Mar 2, 2023

DMRobertson commented Mar 9, 2023

adiroiban commented Mar 9, 2023

DMRobertson commented Mar 10, 2023

adiroiban left a comment

adiroiban Oct 12, 2023

adiroiban Oct 12, 2023

adiroiban Oct 12, 2023

adiroiban Oct 12, 2023

adiroiban Oct 12, 2023

DMRobertson commented Nov 3, 2023

glyph commented Nov 3, 2023

#11812 Mark TerminalRealm as implementing IRealm #11813

#11812 Mark TerminalRealm as implementing IRealm #11813

Conversation

DMRobertson commented Feb 25, 2023 • edited

Scope and purpose

Contributor Checklist:

DMRobertson commented Feb 25, 2023 • edited

DMRobertson commented Feb 25, 2023

glyph commented Feb 25, 2023

adiroiban left a comment • edited

Choose a reason for hiding this comment

exarkun commented Mar 2, 2023

DMRobertson commented Mar 9, 2023

adiroiban commented Mar 9, 2023

DMRobertson commented Mar 10, 2023

adiroiban left a comment

Choose a reason for hiding this comment

adiroiban Oct 12, 2023

Choose a reason for hiding this comment

adiroiban Oct 12, 2023

Choose a reason for hiding this comment

adiroiban Oct 12, 2023

Choose a reason for hiding this comment

adiroiban Oct 12, 2023

Choose a reason for hiding this comment

adiroiban Oct 12, 2023

Choose a reason for hiding this comment

DMRobertson commented Nov 3, 2023

glyph commented Nov 3, 2023

DMRobertson commented Feb 25, 2023 •

edited

DMRobertson commented Feb 25, 2023 •

edited

adiroiban left a comment •

edited